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

Clarify docs around a tag visitor's boolean return value #1479

Merged
merged 1 commit into from
Aug 21, 2024
Merged

Clarify docs around a tag visitor's boolean return value #1479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion plugins/auto-sizes/optimization-detective.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* @since 1.1.0
*
* @param OD_Tag_Visitor_Context $context Tag visitor context.
* @return false Whether the tag should be recorded in URL metrics.
* @return false Whether the tag should be tracked in URL metrics.
*/
function auto_sizes_visit_tag( OD_Tag_Visitor_Context $context ): bool {
Copy link
Member

Choose a reason for hiding this comment

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

@westonruter

which code bits are you referring to?

Haven't looked at the codebase outside this PR, but this function for example would better have a different name now. Of course this is just a plugin consuming Optimization Detective, but it led me to bring this up.

But there's definitely also relevant code in Optimization Detective itself, e.g. the od_register_tag_visitors action could probably use a more accurate name too.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are still tag visitors though. That doesn't change here. What is changed is that tag visitors always visit tags (and they always have) but what is made more clear is what the return value means from a tag visitor. A true return value means that it is indicating that the tag should be tracked/registered/monitored in URL metrics. In the case of the Enhanced Responsive Images plugin, the tag visitor never needs to read the tag out of URL metrics so it always returns false.

Copy link
Member

Choose a reason for hiding this comment

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

But why are they called "tag visitors"? I agree with the rationale for the changes in this PR, but in the same sense I find it equally unclear what "visiting" is about when looking at the API as a whole. "Tracking" a tag makes a lot more sense for me in describing the purpose of why I should be using this API. The purpose, as far as I understand, is to track certain tags - which implies "visiting", but that's not really the purpose and less clear to me.

Copy link
Member Author

@westonruter westonruter Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tag visitors visit every open tag in the document.

Only a subset of tag visitors need to track the tag in URL metrics. If a tag visitor doesn't need URL metrics to perform it's optimizations (as is the case for auto-sizes) then no tracking is happening.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for clarifying!

if ( 'IMG' !== $context->processor->get_tag() ) {
Expand Down
2 changes: 1 addition & 1 deletion plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ final class Embed_Optimizer_Tag_Visitor {
* @since 0.2.0
*
* @param OD_Tag_Visitor_Context $context Tag visitor context.
* @return bool Whether the visit or visited the tag.
* @return bool Whether the tag should be tracked in URL metrics.
*/
public function __invoke( OD_Tag_Visitor_Context $context ): bool {
$processor = $context->processor;
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -23,7 +23,7 @@ final class Image_Prioritizer_Background_Image_Styled_Tag_Visitor extends Image_
* Visits a tag.
*
* @param OD_Tag_Visitor_Context $context Tag visitor context.
* @return bool Whether the visitor visited the tag.
* @return bool Whether the tag should be tracked in URL metrics.
*/
public function __invoke( OD_Tag_Visitor_Context $context ): bool {
$processor = $context->processor;
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -24,7 +24,7 @@ final class Image_Prioritizer_Img_Tag_Visitor extends Image_Prioritizer_Tag_Visi
*
* @param OD_Tag_Visitor_Context $context Tag visitor context.
*
* @return bool Whether the visitor visited the tag.
* @return bool Whether the tag should be tracked in URL metrics.
*/
public function __invoke( OD_Tag_Visitor_Context $context ): bool {
$processor = $context->processor;
Expand Down
2 changes: 1 addition & 1 deletion plugins/image-prioritizer/class-image-prioritizer-tag-visitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ abstract class Image_Prioritizer_Tag_Visitor {
* Visits a tag.
*
* @param OD_Tag_Visitor_Context $context Tag visitor context.
* @return bool Whether the visitor visited the tag.
* @return bool Whether the tag should be tracked in URL metrics.
*/
abstract public function __invoke( OD_Tag_Visitor_Context $context ): bool;

Expand Down
8 changes: 4 additions & 4 deletions plugins/optimization-detective/optimization.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,12 +215,12 @@ function od_optimize_template_output_buffer( string $buffer ): string {
$current_tag_bookmark = 'optimization_detective_current_tag';
$visitors = iterator_to_array( $tag_visitor_registry );
do {
$did_visit = false;
$tracked_in_url_metrics = false;
$processor->set_bookmark( $current_tag_bookmark ); // TODO: Should we break if this returns false?

foreach ( $visitors as $visitor ) {
$cursor_move_count = $processor->get_cursor_move_count();
$did_visit = $visitor( $tag_visitor_context ) || $did_visit;
$cursor_move_count = $processor->get_cursor_move_count();
$tracked_in_url_metrics = $visitor( $tag_visitor_context ) || $tracked_in_url_metrics;

// If the visitor traversed HTML tags, we need to go back to this tag so that in the next iteration any
// relevant tag visitors may apply, in addition to properly setting the data-od-xpath on this tag below.
Expand All @@ -230,7 +230,7 @@ function od_optimize_template_output_buffer( string $buffer ): string {
}
$processor->release_bookmark( $current_tag_bookmark );

if ( $did_visit && $needs_detection ) {
if ( $tracked_in_url_metrics && $needs_detection ) {
$processor->set_meta_attribute( 'xpath', $processor->get_xpath() );
}
} while ( $processor->next_open_tag() );
Expand Down