Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update tag visitor registration with unique ID constants #1469

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
+62 −8
Open

Update tag visitor registration with unique ID constants #1469

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cd8d6b2
Update tag visitor registration with unique ID constants
Aug 13, 2024
4c73618
Update visitor registration and add constant ID
Aug 15, 2024
16b896d
Refactor tag visitor registration to ensure proper dependencies
Aug 15, 2024
1334322
Update plugins/optimization-detective/class-od-tag-visitor-registry.php
pbearne Aug 21, 2024
dbdad08
Update plugins/auto-sizes/optimization-detective.php
pbearne Aug 21, 2024
91be88f
Update plugins/image-prioritizer/helper.php
pbearne Aug 21, 2024
2e640f3
Update plugins/image-prioritizer/helper.php
pbearne Aug 21, 2024
e56c75d
Refactor tag visitor registration to ensure proper dependencies
Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@
*/
final class Image_Prioritizer_Background_Image_Styled_Tag_Visitor extends Image_Prioritizer_Tag_Visitor {

/**
* The ID used the register the class
*/

public const ID = 'image-prioritizer-bg-img';
/**
* Visits a tag.
*
Expand Down
5 changes: 5 additions & 0 deletions plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@
*/
final class Image_Prioritizer_Img_Tag_Visitor extends Image_Prioritizer_Tag_Visitor {

/**
* The ID used the register the class
*/
public const ID = 'image-prioritizer-img';
Copy link
Member

Choose a reason for hiding this comment

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

This would also make sense to add to Embed_Optimizer_Tag_Visitor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, I don't see any change to the Embed_Optimizer_Tag_Visitor class in the Embed Optimizer plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@westonruter I am not sure what is going on here you might need to take this over


/**
* Visits a tag.
*
Expand Down
4 changes: 2 additions & 2 deletions plugins/image-prioritizer/helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ function image_prioritizer_render_generator_meta_tag(): void {
function image_prioritizer_register_tag_visitors( OD_Tag_Visitor_Registry $registry ): void {
// Note: The class is invocable (it has an __invoke() method).
$img_visitor = new Image_Prioritizer_Img_Tag_Visitor();
$registry->register( 'img-tags', $img_visitor );
$registry->register( $img_visitor::ID, $img_visitor );
pbearne marked this conversation as resolved.
Show resolved Hide resolved

$bg_image_visitor = new Image_Prioritizer_Background_Image_Styled_Tag_Visitor();
$registry->register( 'bg-image-tags', $bg_image_visitor );
$registry->register( $bg_image_visitor::ID, $bg_image_visitor );
pbearne marked this conversation as resolved.
Show resolved Hide resolved
}
15 changes: 13 additions & 2 deletions plugins/optimization-detective/class-od-tag-visitor-registry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,20 @@ final class OD_Tag_Visitor_Registry implements Countable, IteratorAggregate {
*
* @param string $id Identifier for the tag visitor.
* @param callable $tag_visitor_callback Tag visitor callback.
* @param array<mixed> $dependencies array with ID as key and Callable as value
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param array<mixed> $dependencies array with ID as key and Callable as value
* @param string[] $dependencies Tag visitors that must run before this tag visitor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel this should be any so we can have more than one dependency

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and that's still the case: string[] is an array of strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the change.

*/
public function register( string $id, callable $tag_visitor_callback ): void {
$this->visitors[ $id ] = $tag_visitor_callback;
public function register( string $id, callable $tag_visitor_callback, array $dependencies = array() ): void
{
if (!empty($dependencies)) {
foreach ($dependencies as $dependency_id => $dependency_callback) {
if (!self::is_registered($dependency_id)) {
$this->visitors[$dependency_id] = $dependency_callback;
}
}
}
if (!self::is_registered($id)) {
$this->visitors[$id] = $tag_visitor_callback;
}
}
Copy link
Member

@westonruter westonruter Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking rather that this would work more like how WP_Scripts works, where you can register a script (or tag visitor) with a given handle (or ID) and supply dependencies which are a list of strings (i.e. handles/IDs). Then later when obtaining the list of tag visitors to apply, it would recursively look up the registered tag visitors to make sure that those which have no dependencies run first, followed by those which have dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see a way of changing this to just passing the className and the dependencies as a series of class names
and then getting the ID from the class

Or just building an array values and then load them in the first call to get_registered

Part of the problem is I am not sure what the code is doing

Copy link
Member

Choose a reason for hiding this comment

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

The tag visitor registry is a collection of callables which are invoked when iterating over the tags via HTML Tag Processor. A tag visitor may be an invokable class or it may be a closure or a function or any other callable. See the Enhanced Responsive Images for an example tag visitor which is a regular PHP function and not a class:

performance/plugins/auto-sizes/optimization-detective.php

Lines 13 to 52 in bc37748

/**
* Visits responsive lazy-loaded IMG tags to ensure they include sizes=auto.
*
* @since 1.1.0
*
* @param OD_Tag_Visitor_Context $context Tag visitor context.
* @return false Whether the tag should be recorded in URL metrics.
*/
function auto_sizes_visit_tag( OD_Tag_Visitor_Context $context ): bool {
if ( 'IMG' !== $context->processor->get_tag() ) {
return false;
}
$sizes = $context->processor->get_attribute( 'sizes' );
if ( ! is_string( $sizes ) ) {
return false;
}
$sizes = preg_split( '/\s*,\s*/', $sizes );
if ( ! is_array( $sizes ) ) {
return false;
}
$is_lazy_loaded = ( 'lazy' === $context->processor->get_attribute( 'loading' ) );
$has_auto_sizes = in_array( 'auto', $sizes, true );
$changed = false;
if ( $is_lazy_loaded && ! $has_auto_sizes ) {
array_unshift( $sizes, 'auto' );
$changed = true;
} elseif ( ! $is_lazy_loaded && $has_auto_sizes ) {
$sizes = array_diff( $sizes, array( 'auto' ) );
$changed = true;
}
if ( $changed ) {
$context->processor->set_attribute( 'sizes', join( ', ', $sizes ) );
}
return false; // Since this tag visitor does not require this tag to be included in the URL Metrics.
}

So in the same way as with WP_Scripts where registered scripts have handles, here too tag visitors (callables) have IDs. As with script dependencies being able to register a script which depends on another script by passing an array of dependency handles, so too as seen in Enhanced Responsive Images there should be a way to register a tag visitor so that it runs after other tag visitors have done.


/**
Expand Down
Loading