Skip to content

Conversation

@tdatu
Copy link
Contributor

@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?

*/
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
Contributor 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.

@aatanasov-cloudinary aatanasov-cloudinary changed the base branch from master to develop July 11, 2025 07:14
Copy link
Collaborator

@aatanasov-cloudinary aatanasov-cloudinary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdatu, Thank you for your contribution. Looks good, I'll adjust the Filter information in another PR. That way, the new filter will appear here.

@aatanasov-cloudinary aatanasov-cloudinary merged commit ba0bc2b into cloudinary:develop Jul 11, 2025
3 checks passed
This was referenced Aug 6, 2025
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.

5 participants