Fire actions before and after Optimization Detective processes a document #1919

westonruter · 2025-03-12T00:44:10Z

This is to address the discussion starting at #1859 (comment).

Two new actions are introduced with this PR:

od_start_template_optimization
od_finish_template_optimization

The first is fired before walking over the document and invoking all of the tag visitors. The second is fired after the processing of the document is complete. Both actions are passed a new OD_Template_Optimization_Context object which contains the following properties:

 * @property-read OD_URL_Metric_Group_Collection $url_metric_group_collection URL Metric group collection.
 * @property-read positive-int|null              $url_metrics_id              ID for the od_url_metrics post which provided the URL Metrics in the collection.
 * @property-read array<string, mixed>           $normalized_query_vars       Normalized query vars.
 * @property-read non-empty-string               $url_metrics_slug            Slug for the od_url_metrics post.
 * @property-read OD_Link_Collection             $link_collection             Link collection.

~~Additionally, two methods are exposed which are wrappers for the corresponding methods in OD_HTML_Tag_Processor:~~

~~::append_head_html( string $html )~~
~~::append_body_html( string $html )~~

Note that the underlying OD_HTML_Tag_Processor is not exposed in this context because at od_start_template_optimization the iteration over the document has not started and at od_finish_template_optimization the iteration is completed. Mutations of specific tags are expected to be done via tag visitors, with the exception of being able to insert links via the supplied OD_Link_Collection instance and these two methods which append raw HTML to the HEAD and BODY.

These actions will allow plugins to do more things with the URL Metrics, like extend the admin bar with URL Metric information without having to re-construct the OD_URL_Metric_Group_Collection. For example, the OD Admin UI plugin can use this to populate the Admin Bar item for URL Metrics without having to re-construct the OD_URL_Metric_Group_Collection: westonruter/od-admin-ui#11

Additionally, the od_start_template_optimization action allows for tag visitors to do some initialization logic based on the current OD_URL_Metric_Group_collection prior to iterating over the document, which otherwise requires this initialization logic to be handled in the tag visitor callback itself, which is awkward. See #1921 for how this improves the tag visitor for background images in Image Prioritizer.

This also allows plugins to conditionally opt-out of caching a pre-optimized page, in particular by allowing them to access the constructed OD_URL_Metric_Group_Collection. For example, WP Super Cache allows plugins to prevent storing a page in cache by defining a DONOTCACHEPAGE constant. So if you wanted to prevent caching a page until all viewport groups are populated, you could do:

add_action(
	'od_finish_template_optimization',
	static function ( OD_Template_Optimization_Context $context ) {
		if (
			! $context->url_metric_group_collection->is_every_group_populated()
		) {
			if ( ! defined( 'DONOTCACHEPAGE' ) ) {
				define( 'DONOTCACHEPAGE', true );
			}
			// TODO: Not supported yet: $context->append_body_html( '<!-- Page prevented from being cached since URL Metrics are not populating every group. -->' );
		} else {
			// TODO: Not supported yet: $context->append_body_html( '<!-- All URL Metric groups are populated! -->' );
		}
	}
);

Or assuming another caching layer like Varnish, you could send the Cache-Control: private header if all groups are not complete, and you want to :

add_action(
	'od_finish_template_optimization',
	static function ( OD_Template_Optimization_Context $context ) {
		if ( ! $context->url_metric_group_collection->is_every_group_complete() ) {
			header( 'Cache-Control: private', true );

			$incomplete_groups = array();
			foreach ( $context->url_metric_group_collection as $group ) {
				if ( ! $group->is_complete() ) {
					$incomplete_groups[] = od_generate_media_query( $group->get_minimum_viewport_width(), $group->get_maximum_viewport_width() );
				}
			}
			// TODO: Not supported yet: $context->append_body_html( '<!-- Incomplete URL Metric groups: ' . join( ', ', $incomplete_groups ) . '-->' );
		}
	}
);

These essentially implement #1912.

Originally I tried to move the initialization of the Optimization Detective objects just before the template was being rendered, but there was a tradeoff here. On one hand, it would allow all of the OD_URL_Metric_Group_Collection to be available earlier so the template could be rendered with information from the collection (e.g. to populate the admin bar). However, the downside here is that this would prevent the ability to register tag visitors conditionally when the page is rendered. For example, it doesn't make a lot of sense to register a tag visitor for a block that doesn't exist in the page. So you could do something like this to conditionally register a tag visitor to set fetchpriority on a SCRIPT tag for embeds, with a value of high if it appears in the initial viewport and low if it does not:

add_filter(
	'render_block_core/embed',
	function ( $content ) {
		if ( ! has_action( 'od_register_tag_visitors', 'register_embed_script_defer_tag_visitor' ) ) {
			add_action( 'od_register_tag_visitors', 'register_embed_script_defer_tag_visitor' );
		}
		return $content;
	}
);

/**
 * Register tag visitor that adds fetchpriority to SCRIPT tags for embeds based on whether they appear in the viewport.
 * 
 * Scripts for embeds in the viewport get fetchpriority=high and those outside the viewport get fetchpriority=low.
 * 
 * @param OD_Tag_Visitor_Registry $tag_visitor_registry
 *
 * @return void
 */
function register_embed_script_defer_tag_visitor( OD_Tag_Visitor_Registry $tag_visitor_registry ) {
	$tag_visitor_registry->register( 'embed_script_lower_priority', static function ( OD_Tag_Visitor_Context $context ) {
		$processor = $context->processor;
		if ( ! $processor->has_class( 'wp-block-embed' ) ) {
			return;
		}

		$context->track_tag();

		// Abort if URL Metrics aren't collected for mobile and desktop yet.
		if (
			$context->url_metric_group_collection->get_first_group()->count() === 0
			&&
			$context->url_metric_group_collection->get_last_group()->count() === 0
		) {
			return;
		}

		$is_visible = $context->url_metric_group_collection->get_element_max_intersection_ratio( $processor->get_xpath() ) > PHP_FLOAT_EPSILON;

		$initial_depth = $processor->get_current_depth();
		while ( $processor->next_tag( array( 'tag_closers' => 'skip' ) ) ) {
			if ( $processor->get_tag() === 'SCRIPT' ) {
				$processor->set_attribute( 'fetchpriority', $is_visible ? 'high' : 'low' );
			}
			if ( $initial_depth === $processor->get_current_depth() ) {
				// Stop when we reach the end of the tag.
				break;
			}
		}
	} );
}

Add tests.

plugins/optimization-detective/optimization.php

…ffer()

…as fired

…ation loop

codecov · 2025-03-13T20:37:49Z

Codecov Report

Attention: Patch coverage is 97.95918% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.39%. Comparing base (ab0b99a) to head (a01d760).
Report is 17 commits behind head on trunk.

Files with missing lines	Patch %	Lines
plugins/optimization-detective/load.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk    #1919      +/-   ##
==========================================
+ Coverage   71.25%   71.39%   +0.13%     
==========================================
  Files          85       86       +1     
  Lines        7006     7043      +37     
==========================================
+ Hits         4992     5028      +36     
- Misses       2014     2015       +1

Flag	Coverage Δ
multisite	`71.39% <97.95%> (+0.13%)`	⬆️
single	`40.43% <0.00%> (-0.22%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2025-03-13T21:23:50Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

felixarntz

@westonruter Overall this looks good to me. A few questions and notes:

I don't fully understand what part of the conversation starting in REST API logic in Optimization Detective can be encapsulated into a ~static~ class #1859 (comment) this is supposed to address. Can you clarify?
To confirm, the main purpose of these actions is to only allow reading relevant data for the OD optimization process right? In other words, they are not meant to modify/perform any actual optimizations?
I'm not sure about the append_head_html and append_body_html methods in their current implementation. Other than appending body comments, what are real use-cases for the two methods?
I know they're documented to not validate and that one needs to be careful for that reason, but (partly dependent on your response to my previous point) we may want to come up with something better. For example, there could be an append_body_comment method that expects a string that is HTML escaped and wrapped in the HTML comment markup. This would be much safer than allowing arbitrary HTML.

felixarntz · 2025-03-13T21:29:46Z

plugins/optimization-detective/optimization.php

+	$query_vars = od_get_normalized_query_vars();
+	$slug       = od_get_url_metrics_slug( $query_vars );
+	$post       = OD_URL_Metrics_Post_Type::get_post( $slug );
+	$post_id    = $post instanceof WP_Post && $post->ID > 0 ? $post->ID : null;


I find this a bit strange - I assume without it PHPStan complains. But in reality a WP_Post should never have a non-positive ID. Obviously an additional check for it is not a real problem, but if we then set the $post_id to null if not, we should also set the $post to null in that same situation.

In other words: If somehow the WP_Post is invalid, we shouldn't allow the post object to get passed through either, if we don't accept its ID value.

Yes, it's for the sake of PHPStan. The condition was present before when constructing OD_Tag_Visitor_Context but just moved up here.

Technically WP_Post can have a negative integer sometimes. It was a hack I used in the Customizer to denote nav_menu_item posts which were in a changeset but not yet published to the database 😅

Otherwise, the $post is only used in OD_URL_Metrics_Post_Type::get_url_metrics_from_post() below, which only looks at the post_content of the object, so in that case it doesn't matter if the ID is not valid.

But if a post doesn't have an ID, wouldn't it be considered irrelevant for Optimization Detective too? I think it would make more sense to then nullify it too for a consistent experience to avoid a weird edge-case.

Since in practice it will never not be a positive-int, I've opted for phpdoc to declare it as such: 0d8cb65.

plugins/optimization-detective/optimization.php

westonruter · 2025-03-13T21:47:45Z

@felixarntz:

I don't fully understand what part of the conversation starting in REST API logic in Optimization Detective can be encapsulated into a ~~static~~ class #1859 (comment) this is supposed to address. Can you clarify?

It addresses it by eliminating the need for global variables or singletons, where the objects are exposed as part of actions instead.

To confirm, the main purpose of these actions is to only allow reading relevant data for the OD optimization process right? In other words, they are not meant to modify/perform any actual optimizations?

Correct, although they can be used to add new preload/preconnect links or append HTML to the HEAD and BODY, which could be relevant for optimization.

I'm not sure about the append_head_html and append_body_html methods in their current implementation. Other than appending body comments, what are real use-cases for the two methods?

See usage in westonruter/od-admin-ui#11. It is used to grab URL Metric data for the plugin to add to the admin bar. You can see it is used to add a stylesheet to the HEAD and a script module to the BODY.

I know they're documented to not validate and that one needs to be careful for that reason, but (partly dependent on your response to my previous point) we may want to come up with something better. For example, there could be an append_body_comment method that expects a string that is HTML escaped and wrapped in the HTML comment markup. This would be much safer than allowing arbitrary HTML.

See the previous point. Validation could be added later via fragments in the HTML Processor, but these methods are exposed now already for tag visitors to append arbitrary HTML to the HEAD and BODY already (e.g. Embed Optimizer uses these to inject styles and scripts as well).

Co-authored-by: Felix Arntz <[email protected]>

felixarntz · 2025-03-13T22:18:23Z

@westonruter

It addresses it by eliminating the need for global variables or singletons, where the objects are exposed as part of actions instead.

Makes sense, thanks 👍

Correct, although they can be used to add new preload/preconnect links or append HTML to the HEAD and BODY, which could be relevant for optimization.

But that doesn't seem to be the purpose of this action? Otherwise I would expect the other plugins like Image Prioritizer to use it for example. I would advise caution against allowing to do things in an action just because they're possible when that's not the main intent of the action.

See usage in westonruter/od-admin-ui#11. It is used to grab URL Metric data for the plugin to add to the admin bar. You can see it is used to add a stylesheet to the HEAD and a script module to the BODY.

[...]

See the previous point. Validation could be added later via fragments in the HTML Processor, but these methods are exposed now already for tag visitors to append arbitrary HTML to the HEAD and BODY already (e.g. Embed Optimizer uses these to inject styles and scripts as well).

Why don't we instead add specific methods for adding script tags, style tags, and HTML comments instead? I think that would be a better solution. I don't like the idea of now allowing any HTML and later adding validation, as that would effectively become a breaking change. Since this is going to be a public API, I think we should go for more specific intents and have validation from the start.

Alternatively, we leave those methods out for now and focus only on the "read" / "access" aspect of this action. Then we can discuss in a follow up issue which write-actions would make sense via this action. This discussion would also go back to my above point - if it's possible via another action, why should it also be possible with this action?

westonruter · 2025-03-13T23:07:26Z

@felixarntz

But that doesn't seem to be the purpose of this action? Otherwise I would expect the other plugins like Image Prioritizer to use it for example. I would advise caution against allowing to do things in an action just because they're possible when that's not the main intent of the action.

Image Prioritizer and Embed Optimizer could indeed use these actions, but they haven't been available. Allowing insertion of HTML once via the od_finish_template_optimization avoids the need for tag visitors to keep track of whether they inserted or not. They can use the tag visitor callbacks to get a "lay of the land" by looking at all of the tags, and then at the od_finish_template_optimization action they can finalize what they need to insert in the HEAD or the BODY.

This could, for example, allow tag visitors to better optimize stylesheets they insert into the document. Instead of Embed Optimizer inserting a separate STYLE for each embed to reserve space to reduce layout shifts, it could instead insert a single STYLE at od_finish_template_optimization which combines all the style rules in one stylesheet. For example, this would allow Embed Optimizer to better group styles by media query instead of having to output @media (width <= 480px) {} for each embed on the page. Currently for each embed it inserts a STYLE like:

<style>
@media (width <= 480px) { #embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; } }
@media (480px < width <= 600px) { #embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; } }
@media (600px < width <= 782px) { #embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; } }
@media (782px < width) { #embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; } }
</style>

With the od_finish_template_optimization action, it could just print the @media at-rules once for each viewport group rather than duplicating them.

Why don't we instead add specific methods for adding script tags, style tags, and HTML comments instead? I think that would be a better solution. I don't like the idea of now allowing any HTML and later adding validation, as that would effectively become a breaking change. Since this is going to be a public API, I think we should go for more specific intents and have validation from the start.

That could be done, but note that ::append_head_html() and ::append_body_html() are already exposed on OD_HTML_Tag_Processor and are used by tag visitors.

The current use cases for ::append_head_html() are:

Optimization Detective injecting the LINK HTML markup returned by OD_Link_Collection::get_html().
Image Prioritizer adding a STYLE tag via a tag visitor to add a style for lazy loaded background images. This could be done via the od_finish_template_optimization action instead, since otherwise the tag visitor needs to keep track with a added_lazy_assets class member variable for whether it inserted the script yet.
Embed Optimizer adding a STYLE tag via a tag visitor to reduce layout shifts.
Content Visibility adding a STYLE tag via a tag visitor for CV styles.

The current use cases for ::append_body_html() are:

Optimization Detective uses this to insert the detect.js script to the page.
Embed Optimizer adding a SCRIPT to the end of the BODY when there is a lazy-loaded embed on the page. This could be done via the od_finish_template_optimization action instead, since otherwise the tag visitor needs to keep track with a added_lazy_script class member variable for whether it inserted the script yet.
Image Prioritizer adding a SCRIPT tag via a tag visitor to lazy load background images. This could be done via the od_finish_template_optimization action instead, since otherwise the tag visitor needs to keep track with a added_lazy_assets class member variable for whether it inserted the script yet.
Image Prioritizer adding a SCRIPT tag via a tag visitor to lazy load videos. This could be done via the od_finish_template_optimization action instead, since otherwise the tag visitor needs to keep track with a added_lazy_script class member variable for whether it inserted the script yet. Example: Use od_finish_template_optimization action to insert video lazy-load script #1922.

So there would need to be new methods like:

::append_head_style() to append an inline stylesheet
::append_head_link() (or as ::append_head_links() it could be passed an OD_Link_Collection)
::append_head_script() (but also differentiate between inline vs non-inline?)
::append_body_script() (ditto)
::append_body_link() (not used yet, but useful for adding non-blocking external stylesheets)

I think it's useful to have methods that allow inserting arbitrary raw HTML in the HEAD and BODY, for example to insert a META tag in the HEAD or other use cases I haven't thought of. Adding all of these methods now I think would add too much scope to this PR. Perhaps the phpdoc for the methods can say that it is the responsibility of the extension to ensure well-formed HTML is supplied, and that in the future validation may be enforced with the HTML Processor.

Alternatively, we leave those methods out for now and focus only on the "read" / "access" aspect of this action. Then we can discuss in a follow up issue which write-actions would make sense via this action. This discussion would also go back to my above point - if it's possible via another action, why should it also be possible with this action?

Because not all document modifications are directly related to a tag visitor. For example, with the OD Admin UI plugin, it inserts markup based on the current collection of URL Metrics, regardless of any tags in the page.

westonruter · 2025-03-13T23:11:34Z

plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php

@@ -143,6 +143,7 @@ private function get_common_lcp_element_external_background_image( OD_URL_Metric
 	private function maybe_preload_external_lcp_background_image( OD_Tag_Visitor_Context $context ): void {
 		// Gather the tuples of URL Metric group and the common LCP element external background image.
 		// Note the groups of URL Metrics do not change across invocations, we just need to compute this once for all.
+		// TODO: Instead of populating this here, it could be done once per invocation during the od_start_template_optimization action since the page's OD_URL_Metric_Group_Collection is available there.


felixarntz · 2025-03-14T15:04:13Z

@westonruter Thanks for outlining some more use-cases for this. As far as I understand though, several of them are already possible via the tag visitors today - which goes back to my concern about also allowing it here.

But my main question is: Why are none of these use-cases being implemented here? That would provide a better foundation to discuss the implementation of the "write" methods. I'm still not convinced about the openness of these methods and whether a "context" object should have them.

At the end of the day, this needs more discussion with real examples to consider how these methods would be used. For example, based on your comment that the extension plugins could use these actions to append HTML once instead of doing it in the tag visitor, maybe this should rather go hand in hand with another change to disallow doing so in the tag visitor.

Given this is milestoned for Monday's release, we have almost no time left. I think it would be best to downscope this PR to focus only on introducing the actions and the template context class for context awareness, but leave out any write methods. Since there is no usage of them in neither of the plugins, I don't see a reason these have to be added in now. We could discuss them in a follow-up issue with more time on our hands.

westonruter · 2025-03-14T15:39:23Z

@felixarntz:

But my main question is: Why are none of these use-cases being implemented here? That would provide a better foundation to discuss the implementation of the "write" methods. I'm still not convinced about the openness of these methods and whether a "context" object should have them.

The reason is that since Image Prioritizer and Embed Optimizer depend on Optimization Detective. If they were part of this release, then all plugins would have to be updated at the same time. I'd rather get the new fundamental API out as part of this 1.0.0-beta3 release so that for the 1.0.0 final we can safely update Image Prioritizer and Embed Optimizer to make use of them. You can see there are use cases being implemented:

This seems better than introducing a new fundamental API in 1.0.0 without a beta first.

If the due date is the problem we can just move it back, as we discussed these releases being more ad hoc anyway.

felixarntz · 2025-03-14T16:03:24Z

@westonruter

The reason is that since Image Prioritizer and Embed Optimizer depend on Optimization Detective. If they were part of this release, then all plugins would have to be updated at the same time.

I don't think that's a good reason to not make the change at the same time. If they need to be updated at the same time, the problem would be backward compatibility, not timing.

This seems better than introducing a new fundamental API in 1.0.0 without a beta first.

The fundamental API would be introduced here - the hooks and the context object. Adding more methods to it would be an enhancement, and that is totally reasonable for post-1.0.0 IMO.

If the due date is the problem we can just move it back, as we discussed these releases being more ad hoc anyway.

Per the above, I don't see why that's necessary. There are sufficient changes in the plugins already to do a release now as we had planned. If you think we are not ready for Optimization Detective 1.0.0 yet as the next release, we could also have another 1.0.0-beta4 release.

westonruter · 2025-03-14T16:11:20Z

@felixarntz

I don't think that's a good reason to not make the change at the same time. If they need to be updated at the same time, the problem would be backward compatibility, not timing.

Right, it's not a back-compat problem. Image Prioritizer and Embed Optimizer will still work as expected without updating to make use of this new API. I was confused.

The fundamental API would be introduced here - the hooks and the context object. Adding more methods to it would be an enhancement, and that is totally reasonable for post-1.0.0 IMO.

Still, since there is a clear need for these hooks I want to include them prior to the 1.0.0 release. It address something that always nagged at me about the API but I didn't have a solution for it until now.

felixarntz · 2025-03-14T16:14:43Z

@westonruter

Still, since there is a clear need for these hooks I want to include them prior to the 1.0.0 release. It address something that always nagged at me about the API but I didn't have a solution for it until now.

Then how about this? We could

remove the write methods from this PR
ship the remainder (basically actions and context object) in 1.0.0-beta3 next week
focus on figuring out the write methods, plus their usage in the extensions for 1.0.0-beta4 (and the respective latest extension betas)
then do a cycle of only bug fixes (if any) before 1.0.0 stable

westonruter · 2025-03-14T16:26:59Z

@felixarntz

Right, it's not a back-compat problem. Image Prioritizer and Embed Optimizer will still work as expected without updating to make use of this new API. I was confused.

Oh nevermind, I wasn't confused. The problem is that if someone were to update Image Prioritizer and Embed Optimizer which used the new hooks, but didn't update Optimization Detective. Then they would stop working.

focus on figuring out the write methods, plus their usage in the extensions for 1.0.0-beta4 (and the respective latest extension betas)

Given the above, I think we'll need to be sure to release the hooks earlier so ensure users update OD so they are available.

I don't see the need to omit append_head_html() and append_body_html() from this beta, given these are already exposed on the context provided to tag visitors. If they are problematic then they'll need to be deprecated everywhere and not now in beta. It makes more sense to me to introduce tailored HTML insertion methods for a 1.1.0 release.

felixarntz · 2025-03-14T18:31:00Z

@westonruter

Oh nevermind, I wasn't confused. The problem is that if someone were to update Image Prioritizer and Embed Optimizer which used the new hooks, but didn't update Optimization Detective. Then they would stop working.

But then this is a BC break, which shouldn't happen in the first place. It should be possible to have (at least temporarily) conditionals in Image Prioritizer and Embed Optimizer so that they use the most suitable way that's available via the Optimization Detective version used.

Given the above, I think we'll need to be sure to release the hooks earlier so ensure users update OD so they are available.

See above, I don't think this is a good idea. We can't rely on timing, there should be BC measures in place instead.

I don't see the need to omit append_head_html() and append_body_html() from this beta, given these are already exposed on the context provided to tag visitors. If they are problematic then they'll need to be deprecated everywhere and not now in beta.

I disagree. It's not just about whether direct HTML modification is generally the right way, it's also about whether it should happen in this hook vs in the other hook. So it requires a more nuanced conversation where we consider the whole API, not just this new action hook.

It makes more sense to me to introduce tailored HTML insertion methods for a 1.1.0 release.

That would work for me.

westonruter · 2025-03-14T18:33:43Z

@felixarntz

But then this is a BC break, which shouldn't happen in the first place. It should be possible to have (at least temporarily) conditionals in Image Prioritizer and Embed Optimizer so that they use the most suitable way that's available via the Optimization Detective version used.

This is why I'm not including the changes to Embed Optimizer and Image Prioritizer in this PR.

I disagree. It's not just about whether direct HTML modification is generally the right way, it's also about whether it should happen in this hook vs in the other hook. So it requires a more nuanced conversation where we consider the whole API, not just this new action hook.

I think we've had a pretty nuanced conversation here!

felixarntz · 2025-03-14T18:42:42Z

@westonruter

But then this is a BC break, which shouldn't happen in the first place. It should be possible to have (at least temporarily) conditionals in Image Prioritizer and Embed Optimizer so that they use the most suitable way that's available via the Optimization Detective version used.

This is why I'm not including the changes to Embed Optimizer and Image Prioritizer in this PR.

But that doesn't make it a non-breaking change then. If we need to break BC, there should always be at least 1-2 releases where both the old and new way are supported, with the old one triggering deprecation warnings, or (in this case possibly more likely) add a warning to update to the newer OD version. Alternatively, we would need to bump the minimum requirement for the OD version in each extension right away, which means there's no need for temporary workarounds, but the feature would be disabled (temporarily) if a site only updates the extensions but not OD.

I disagree. It's not just about whether direct HTML modification is generally the right way, it's also about whether it should happen in this hook vs in the other hook. So it requires a more nuanced conversation where we consider the whole API, not just this new action hook.

I think we've had a pretty nuanced conversation here!

True, but we haven't discussed yet for example why injecting head and/or body HTML should be possible both via the tag visitor context and the template context - to me that's confusing from an API perspective.

I think we should continue discussing that, but the more time-sensitive decision is what to do about this upcoming release. I still don't understand what is the rush in including the write methods in this releases, rather than figuring this out "calmly" without the time pressure and implement what we decide on for a 1.0.0-beta4 (ideally also accompanied by the usage of it in the extension plugins).

westonruter · 2025-03-14T18:59:39Z

@felixarntz

But that doesn't make it a non-breaking change then. If we need to break BC, there should always be at least 1-2 releases where both the old and new way are supported, with the old one triggering deprecation warnings, or (in this case possibly more likely) add a warning to update to the newer OD version. Alternatively, we would need to bump the minimum requirement for the OD version in each extension right away, which means there's no need for temporary workarounds, but the feature would be disabled (temporarily) if a site only updates the extensions but not OD.

Yes, Image Prioritizer and Embed Optimizer can be updated to short-circuit at the od_init action and show an upgrade notice once they have been updated to use the new hooks.

True, but we haven't discussed yet for example why injecting head and/or body HTML should be possible both via the tag visitor context and the template context - to me that's confusing from an API perspective.

There are three scenarios for inserting markup:

Before the document has processed at od_start_template_optimization. This is where a plugin like OD Admin UI can add styles or scripts that just require information about the OD_URL_Metric_Group_Collection and don't depend on the outcome of tag visitors being invoked (Leverage OD_Template_Optimization_Context westonruter/od-admin-ui#11). However, this could just as well be done at od_finish_template_optimization which is passed the same context. The more important use case for od_start_template_optimization is for tag visitors to do some pre-processing on the URL Metrics to gather up the information they'll need later when walking over the tags, e.g. Leverage od_start_template_optimization action to initialize common LCP external background image data #1921
During a tag visitor's callback, which before now has been the only option. This remains a fine option to insert an additional script in the page, for example when a tag visitor comes across a video that should be lazy-loaded, it can append a SCRIPT to the BODY and then set a flag to prevent adding a second script when another lazy-loaded video is encountered. Alternatively, the od_finish_template_optimization action can be used for this instead, although it essentially needs the same flag regardless: Use od_finish_template_optimization action to insert video lazy-load script #1922.
After the document has been processed at od_finish_template_optimization. At this point, all tag visitors have run and extensions have an overall lay of the land for what is in the document. They can now compile a single stylesheet for insertion in the HEAD based on the information gathered by the tag visitors, for example, as seen in De-duplicate media queries in Embed Optimizer by merging style rules and inserting after optimizing the document #1923.

I think we should continue discussing that, but the more time-sensitive decision is what to do about this upcoming release. I still don't understand what is the rush in including the write methods in this releases, rather than figuring this out "calmly" without the time pressure and implement what we decide on for a 1.0.0-beta4 (ideally also accompanied by the usage of it in the extension plugins).

I was hoping to be able to avoid a beta4. If this is included in beta3, then extension plugins can make use of the new APIs at the time of 1.0.0. Even if we do have a beta4, then this being included now will allow us to have more time to make sure the it is able to be leveraged by extensions effectively, but I think this has been already demonstrated with the above PRs.

felixarntz · 2025-03-14T19:32:17Z

@westonruter

Yes, Image Prioritizer and Embed Optimizer can be updated to short-circuit at the od_init action and show an upgrade notice once they have been updated to use the new hooks.

👍 So I think this would mean we can implement the API enhancements and their usage at the same time, and they can be released at the same time.

There are three scenarios for inserting markup:

[...]

Thanks for outlining those. Particularly the 2nd one I find fuzzy. Some thoughts:

There seems to be no benefit in doing it in one or the other way. Given the other scenarios can work with one of the new actions, this leads me to think that we should deprecate adding HTML on the tag visitor context then, to have a clear API on how this should be done.
Since only adding a certain script or stylesheet once is a real concern, this is ideally something the Optimization Detective APIs allow to handle more intuitively. More specific methods for external scripts, inline scripts, external stylesheets, and inline stylesheets (as we discussed earlier) would come in handy for that. We could have these methods require identifiers and then they would behave more like "enqueue" rather than "print", so that each identifier will only be included once even if the method is called for it 20 times.

I was hoping to be able to avoid a beta4.

Not sure why, the reasons you provide seem a bit like a moot point. I think it's totally fine to have a beta4. WordPress Core has had beta4s before too, and FWIW I don't think there needs to be any arbitrary limit on the number of betas a product has. Technically speaking, we've been still refining the API throughout the betas, which isn't something a regular beta would allow for anyway (beyond bug fixes). I'd rather have a 1.0.0 people can trust than introducing methods in the last 1.0.0 beta that may be deprecated again already in a 1.1.0 shortly after.

I think I'm aligned on allowing write methods on the template context, but per my above and previous feedback this requires more thought on how it should be implemented exactly (I'm still not a fan of allowing arbitrary HTML in those methods until there is a realistic use-case for requiring that) and what it means for the tag visitor context (we shouldn't allow to do the same thing on either).

Based on that, I still think the best decision would be to only include the read methods in this release, and continue discussing the above in a follow-up issue which we'll milestone for 1.0.0-beta4.

westonruter · 2025-03-14T20:07:35Z

@felixarntz

👍 So I think this would mean we can implement the API enhancements and their usage at the same time, and they can be released at the same time.

Yes, but if someone updates Image Prioritizer or Embed Optimizer without Optimization Detective then the functionality won't work and they'll get an admin notice. That's fine I guess.

There seems to be no benefit in doing it in one or the other way. Given the other scenarios can work with one of the new actions, this leads me to think that we should deprecate adding HTML on the tag visitor context then, to have a clear API on how this should be done.

The intended design of the tag visitors is to be able to allow one to be written as simply as possible, just by registering a single tag visitor callback. This is possible if there is a way to write to the document from the tag visitor. For example:

add_action( 'od_register_tag_visitors', static function ( OD_Tag_Visitor_Registry $registry ) {
	$registry->register( 'video-lazy-load', static function ( OD_Tag_Visitor_Context $context ) {
		static $added_lazy_load_script = false;

		$processor = $context->processor;
		if ( $processor->get_tag() !== 'VIDEO' ) {
			return;
		}
		$context->track_tag();

		// Abort if URL Metrics aren't collected for mobile and desktop yet.
		if (
			$context->url_metric_group_collection->get_first_group()->count() === 0
			&&
			$context->url_metric_group_collection->get_last_group()->count() === 0
		) {
			return;
		}

		if ( $context->url_metric_group_collection->get_element_max_intersection_ratio( $processor->get_xpath() ) < PHP_FLOAT_EPSILON ) {
			$processor->set_attribute( 'preload', 'none' );
			if ( ! $added_lazy_load_script ) {
				$processor->append_body_html( wp_get_script_tag( array( 'src' => plugins_url( 'lazy-load.js', __FILE__ ) ) ) );
				$added_lazy_load_script = true;
			}
		}
	} );
} );

If writing to the document isn't possible from the tag visitor, then you have to add higher level abstractions, like a class or else a global variable, and then to also add a od_finish_template_optimization action handler to do the insertion. I want it to be possible to keep the logic all self-contained in a single tag visitor, but to also have the option for more refined implementations to use a higher level abstraction (e.g. #1923).

Not sure why, the reasons you provide seem a bit like a moot point. I think it's totally fine to have a beta4. WordPress Core has had beta4s before too, and FWIW I don't think there needs to be any arbitrary limit on the number of betas a product has. Technically speaking, we've been still refining the API throughout the betas, which isn't something a regular beta would allow for anyway (beyond bug fixes). I'd rather have a 1.0.0 people can trust than introducing methods in the last 1.0.0 beta that may be deprecated again already in a 1.1.0 shortly after.

This goes back to the chat during the previous release, where there was confusion about having betas. So I had intended to get away from beta releases, but since this next release is a beta which I hadn't intended either, I guess my goals here are awash. We can add more betas. But perhaps the next beta can be in two weeks rather than waiting a whole month, as we discussed.

Based on that, I still think the best decision would be to only include the read methods in this release, and continue discussing the above in a follow-up issue which we'll milestone for 1.0.0-beta4.

I'd rather not but I will if you insist.

…late_Optimization_Context for now Co-authored-by: Felix Arntz <[email protected]>

felixarntz · 2025-03-14T20:41:52Z

@westonruter

If writing to the document isn't possible from the tag visitor, then you have to add higher level abstractions, like a class or else a global variable, and then to also add a od_finish_template_optimization action handler to do the insertion. I want it to be possible to keep the logic all self-contained in a single tag visitor, but to also have the option for more refined implementations to use a higher level abstraction (e.g. #1923).

Thanks, it makes sense to be able to do it in both ways then. Still, I think there may be a cleaner implementation for that than to expose the same methods on multiple API classes. Maybe we can have those shared write methods on one of the classes that is used across both actions, or on a new class that is then used across both actions, e.g. as a child property of the respective main context class, like $tag_visitor_context->html_writer->append_*() and $template_context->html_writer->append_*() (please discard the naming, this is just to visualize).

My other remaining concern is about allowing to inject raw HTML. This is of course already available via the tag visitor today (which I'm not sure we should allow either), but since this PR would be adding it to another class, I think we should revisit before committing to it for 1.0.0.

We can add more betas. But perhaps the next beta can be in two weeks rather than waiting a whole month, as we discussed.

Yeah, I would prefer to continue the above discussion over the next week and for now merge this without the append methods for beta3. And definitely, no need to wait a month for the following beta4 based on our new approach.

felixarntz

Thanks @westonruter, LGTM!

westonruter · 2025-03-17T00:46:44Z

@felixarntz

I would prefer to continue the above discussion over the next week and for now merge this without the append methods for beta3

See #1931 for the follow-up.

westonruter added this to the optimization-detective 1.0.0-beta3 milestone Mar 12, 2025

westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Mar 12, 2025

westonruter changed the title ~~Move OD initialization to wp action instead of output buffer callback~~ Move Optimization Detective initialization earlier to wp action instead of late in the output buffer callback Mar 12, 2025

westonruter commented Mar 12, 2025

View reviewed changes

plugins/optimization-detective/optimization.php Outdated Show resolved Hide resolved

westonruter force-pushed the update/od-initialization-timing branch from 8e31a06 to f973719 Compare March 12, 2025 18:16

westonruter mentioned this pull request Mar 12, 2025

Leverage OD_Template_Optimization_Context westonruter/od-admin-ui#11

Draft

westonruter added 8 commits March 13, 2025 09:03

Move OD initialization to wp action instead of output buffer callback

abdefc0

Add od_start_template_optimization action

64d63da

Initiaize OD objects at template_include filter

3b0f696

Introduce OD_Template_Optimization_Context

f7da494

Add link collection to context

51feaad

Reuse template optimization context in od_optimize_template_output_bu…

0ad7f79

…ffer()

Finalize the tag registry after the od_register_tag_visitors action h…

8bcfc13

…as fired

Add TODO in Image_Prioritizer_Background_Image_Styled_Tag_Visitor

a57391d

westonruter force-pushed the update/od-initialization-timing branch from 1e8db6d to a57391d Compare March 13, 2025 16:03

Go back to initializing in footer but add action before/after optimiz…

6abee9c

…ation loop

westonruter mentioned this pull request Mar 13, 2025

Leverage od_start_template_optimization action to initialize common LCP external background image data #1921

Draft

westonruter marked this pull request as ready for review March 13, 2025 21:23

westonruter requested a review from felixarntz as a code owner March 13, 2025 21:23

westonruter changed the title ~~Move Optimization Detective initialization earlier to wp action instead of late in the output buffer callback~~ Fire actions before and after Optimization Detective processes a document Mar 13, 2025

westonruter mentioned this pull request Mar 13, 2025

Page caching plugins should have access to helper function for whether fresh URL Metrics are collected #1912

Closed

felixarntz reviewed Mar 13, 2025

View reviewed changes

Rename variable make it distinct from other context

09d7af7

Co-authored-by: Felix Arntz <[email protected]>

westonruter mentioned this pull request Mar 13, 2025

Use od_finish_template_optimization action to insert video lazy-load script #1922

Draft

Use phpdoc to declare that the post ID is a positive-int|null

0d8cb65

westonruter commented Mar 13, 2025

View reviewed changes

Add tests and fix bug with adding links at finish action

38a4c6b

westonruter requested a review from felixarntz March 14, 2025 00:13

Add docs for requirement for valid HTML passed into methods

5d12ef7

westonruter mentioned this pull request Mar 14, 2025

Add ability to prime URL metrics #1850

Open

1 task

Merge branch 'trunk' into update/od-initialization-timing

5da134c

westonruter mentioned this pull request Mar 14, 2025

De-duplicate media queries in Embed Optimizer by merging style rules and inserting after optimizing the document #1923

Draft

Remove append_head_html() and append_body_html() methods from OD_Temp…

53a91a2

…late_Optimization_Context for now Co-authored-by: Felix Arntz <[email protected]>

Merge branch 'trunk' into update/od-initialization-timing

a01d760

westonruter mentioned this pull request Mar 15, 2025

Try: Use WP_HTML_Processor in Optimization Detective #1546

Draft

5 tasks

felixarntz approved these changes Mar 15, 2025

View reviewed changes

westonruter merged commit 47eef00 into trunk Mar 15, 2025
16 checks passed

westonruter deleted the update/od-initialization-timing branch March 15, 2025 02:22

westonruter mentioned this pull request Mar 17, 2025

Methods for inserting HTML should be exposed on OD_Template_Optimization_Context for before or after all tags have been visited #1931

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fire actions before and after Optimization Detective processes a document #1919

Fire actions before and after Optimization Detective processes a document #1919

westonruter commented Mar 12, 2025 •

edited

Loading

codecov bot commented Mar 13, 2025 •

edited

Loading

github-actions bot commented Mar 13, 2025 •

edited

Loading

felixarntz left a comment

felixarntz Mar 13, 2025

westonruter Mar 13, 2025

felixarntz Mar 13, 2025

westonruter Mar 13, 2025

westonruter commented Mar 13, 2025

felixarntz commented Mar 13, 2025 •

edited

Loading

westonruter commented Mar 13, 2025 •

edited

Loading

westonruter Mar 13, 2025

felixarntz commented Mar 14, 2025 •

edited

Loading

westonruter commented Mar 14, 2025

felixarntz commented Mar 14, 2025 •

edited

Loading

westonruter commented Mar 14, 2025

felixarntz commented Mar 14, 2025 •

edited

Loading

westonruter commented Mar 14, 2025

felixarntz commented Mar 14, 2025

westonruter commented Mar 14, 2025 •

edited

Loading

felixarntz commented Mar 14, 2025 •

edited

Loading

westonruter commented Mar 14, 2025

felixarntz commented Mar 14, 2025

westonruter commented Mar 14, 2025 •

edited

Loading

felixarntz commented Mar 14, 2025 •

edited

Loading

felixarntz left a comment

westonruter commented Mar 17, 2025

Fire actions before and after Optimization Detective processes a document #1919

Fire actions before and after Optimization Detective processes a document #1919

Conversation

westonruter commented Mar 12, 2025 • edited Loading

codecov bot commented Mar 13, 2025 • edited Loading

Codecov Report

github-actions bot commented Mar 13, 2025 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz Mar 13, 2025

Choose a reason for hiding this comment

westonruter Mar 13, 2025

Choose a reason for hiding this comment

felixarntz Mar 13, 2025

Choose a reason for hiding this comment

westonruter Mar 13, 2025

Choose a reason for hiding this comment

westonruter commented Mar 13, 2025

felixarntz commented Mar 13, 2025 • edited Loading

westonruter commented Mar 13, 2025 • edited Loading

westonruter Mar 13, 2025

Choose a reason for hiding this comment

felixarntz commented Mar 14, 2025 • edited Loading

westonruter commented Mar 14, 2025

felixarntz commented Mar 14, 2025 • edited Loading

westonruter commented Mar 14, 2025

felixarntz commented Mar 14, 2025 • edited Loading

westonruter commented Mar 14, 2025

felixarntz commented Mar 14, 2025

westonruter commented Mar 14, 2025 • edited Loading

felixarntz commented Mar 14, 2025 • edited Loading

westonruter commented Mar 14, 2025

felixarntz commented Mar 14, 2025

westonruter commented Mar 14, 2025 • edited Loading

felixarntz commented Mar 14, 2025 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

westonruter commented Mar 17, 2025

westonruter commented Mar 12, 2025 •

edited

Loading

codecov bot commented Mar 13, 2025 •

edited

Loading

github-actions bot commented Mar 13, 2025 •

edited

Loading

felixarntz commented Mar 13, 2025 •

edited

Loading

westonruter commented Mar 13, 2025 •

edited

Loading

felixarntz commented Mar 14, 2025 •

edited

Loading

felixarntz commented Mar 14, 2025 •

edited

Loading

felixarntz commented Mar 14, 2025 •

edited

Loading

westonruter commented Mar 14, 2025 •

edited

Loading

felixarntz commented Mar 14, 2025 •

edited

Loading

westonruter commented Mar 14, 2025 •

edited

Loading

felixarntz commented Mar 14, 2025 •

edited

Loading