-
Notifications
You must be signed in to change notification settings - Fork 29
add ability to remove dns-prefetch or preconnect link tags #1023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@tharsheblows, please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atdcloud I've left a comment -- it's not necessarily a blocker but will make the filter a bit easier to use and understand.
Is there somewhere where we should add documentation about the filter?
php/class-delivery.php
Outdated
@@ -765,6 +765,8 @@ protected function setup_hooks() { | |||
*/ | |||
public function dns_prefetch( $urls, $relation_type ) { | |||
|
|||
$relation_type = apply_filters('cld_dns_prefetch', $relation_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 note
This will work but I think the way to do it is something like below. It keeps it a bit more understandable when looking at the code.
...
$dns_prefetch_types = apply_filters( 'cld_dns_prefetch_types', array ( 'dns-prefetch', 'preconnect' ) );
if ( in_array( $relation_type, $dns_prefetch_types, true ) {
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm...I think the IF clause that evaluates the $relation_type is already a dead giveaway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do a couple of things here:
- Avoid using the words
prefetch_types
in filter and variable names, as it's a misnomer. Let's useresource_hints
. - I support using
in_array
to explicitly search through an array instead of mutating$relation_type
. In fact, mutating it has no purpose, as the$relation_type
sets the context for the entire method and should, in my opinion, be treated as immutable.
...
$resource_hints = apply_filters( 'cld_resource_hints', array ( 'dns-prefetch', 'preconnect' ) );
if ( in_array( $relation_type, $resource_hints, true ) {
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and we need to add a hook comment above the new hook in order to make sure the generated documentation is correctly created.
Issue: #1022
Approach
Implemented apply_filters in setup_hooks
QA notes
Simple hooks and implemented in test environment.