Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tdatu
Copy link

@tdatu tdatu commented Mar 20, 2025

Issue: #1022

Approach

Implemented apply_filters in setup_hooks

QA notes

Simple hooks and implemented in test environment.

@atdcloud atdcloud requested a review from tharsheblows March 23, 2025 15:14
@atdcloud
Copy link

@tharsheblows, please check.

Copy link
Contributor

@tharsheblows tharsheblows left a 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?

@@ -765,6 +765,8 @@ protected function setup_hooks() {
*/
public function dns_prefetch( $urls, $relation_type ) {

$relation_type = apply_filters('cld_dns_prefetch', $relation_type);
Copy link
Contributor

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 ) { 
...

Copy link
Author

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.

Copy link
Contributor

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:

  1. Avoid using the words prefetch_types in filter and variable names, as it's a misnomer. Let's use resource_hints.
  2. 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 ) { 
...

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants