REST API logic in Optimization Detective can be encapsulated into a static class

[westonruter]

Feature Description

Optimization Detective currently has static classes for OD_Storage_Lock and OD_URL_Metrics_Post_Type, but it has a rest-api.php includes file with global functions and constants. The REST API logic would make sense to also put into a static class so there is consistency. It would also help with reducing the API surface for functions not needing to be public.

[hbhalodia]

Hi @westonruter, If I had correctly understood, we need to update the file rest-api.php to class-rest-api.php and add all constants, action functions to static class variables and methods?

[westonruter]

Yes, to class-od-rest-api.php. Also, add an add_hooks() method which can be used in hooks.php instead of directly referencing the methods in adding hooks there. This was done for the storage lock and post type classes as well.

[hbhalodia]

Yes, thanks for the clarification, I would proceed with the changes and would update once done.

[westonruter]

Previous reference to this issue: #1098 (comment)

[westonruter]

I've merged #1863 so you can now branch off of trunk and not get merge conflicts.

[felixarntz]

Curious why we should use a static class instead of regular class that is instantiated. The Core endpoints are all written in the latter way.

[westonruter]

It doesn't have to be static but there's not really any value in it not being static. The classes for the storage lock and post type are also static.

[felixarntz]

But there's also no value in being static :)

Instances have a general advantage of being easier to test in case they have any state, because you can just create a new instance for every test. With static classes, you would need workarounds for this kind of thing. This may not be a huge concern depending on what the class does here, but worth considering.

[westonruter]

The value in being static is that you don't introduce a variable that isn't used. If you want to unhook something, you can use the class name. Otherwise, you have to hope there is an instance of the class in a global variable.

[felixarntz]

The question to ask ourselves here is whether we want to allow unhooking what this class does. Obviously, technically everything can be unhooked via some kind of hack (similar how one could even access private PHP methods via hacking with ReflectionClass), but using an instance that is not exposed globally for example implicitly discourages unhooking, which can be for a good reason.

In this particular case, I question why unhooking the REST API logic would be useful. Allowing to unhook everything just for the sake of it is not a great pattern, and unhooking the REST API logic from Optimization Detective would simply break Optimization Detective.

[westonruter]

Maybe you want to override the endpoint with something else for testing purposes or experimental purposes. Like Gutenberg unhooks things in core when it is introducing a new version of something.

In any case, we should be consistent with what is currently in the codebase:

performance/plugins/optimization-detective/hooks.php

Lines 20 to 21 in d21b74d

	OD_URL_Metrics_Post_Type::add_hooks();
	OD_Storage_Lock::add_hooks();

If we want to use a non-static class then the other classes should be updated as well. In this case, would these lines become:

( new OD_URL_Metrics_Post_Type() )->add_hooks(); 
( new OD_Storage_Lock() )->add_hooks(); 

Or would we put these in new global variables?

$od_url_metrics_post_type = new OD_URL_Metrics_Post_Type();
$od_url_metrics_post_type->add_hooks(); 
$od_storage_lock = new OD_Storage_Lock();
$od_storage_lock->add_hooks();

[felixarntz]

Fair point, I agree whatever we go with should be consistent throughout the plugin.

My personal preference would be the ( new OD_URL_Metrics_Post_Type() )->add_hooks(); variant, though the variant with globals would be more in line with how WordPress Core works for the most part.

Since even in Core though, introducing new globals is no longer encouraged, I think it depends:

• If we need to access those class instances from elsewhere in Optimization Detective without passing them in the constructor, we could use functions where they're instantiated in a static variable (e.g. a od_url_metrics_post_type() function that instantiates it when it's called) - almost like a singleton, just better because it doesn't make the constructor private.
• If we don't need to access those class instances from elsewhere, we could just go with the anonymous instantiation.

At the end of the day, I don't feel very strongly about this, despite being in favor of the above. Since we're already in 1.0.0-bet though, we should also consider whether these APIs are currently marked private or not. If not, it would feel wrong to change the APIs now in this way as it would be a huge BC break and would ideally have been done in a 0.x version.

WDYT?

[westonruter]

we should also consider whether these APIs are currently marked private or not. If not, it would feel wrong to change the APIs now in this way as it would be a huge BC break and would ideally have been done in a 0.x version.

These APIs are all private, yes. Both OD_URL_Metrics_Post_Type and OD_Storage_Lock are private, and all the functions and constants in rest-api.php have @access private as well. So we're free to change these however we see fit.

That said, even though they are private, I am using them in the Admin UI plugin (which I don't think is a problem since it is not publicized). Also this is a somewhat experimental extension to core OD itself and not intended as a model for others to follow.

These are the currently-private symbols I'm using in that plugin:

• od_get_breakpoint_max_widths()
• od_get_url_metrics_breakpoint_sample_size()
• od_get_url_metric_freshness_ttl()
• od_can_optimize_response()
• od_get_url_metrics_slug()
• od_get_normalized_query_vars()
• od_get_current_url_metrics_etag()
• od_get_current_theme_template()
• OD_URL_Metrics_Post_Type

Some of these would make sense to be public. But others wouldn't necessarily have to be.

For example, in two places in the frontend context I'm manually instantiating the OD_Tag_Visitor_Registry and OD_URL_Metric_Group_Collection (well, in one but I could in the other as well). (There's also a place I'm instantiating OD_URL_Metric_Group_Collection in the admin.) In the frontend context this is computed after when the buffer is being processed. It could be computed earlier when we add the hook to process the buffer in od_maybe_add_template_output_buffer_filter(). At this point we could make the OD_URL_Metric_Group_Collection instance available as a global without other extensions needing to reconstruct it. But would we make this a new $od_url_metric_group_collection global? Would we have a od_url_metric_group_collection() function that has a static variable like you said? But being a static variable then you can't clear it out. Compare with wp_scripts() which sets the $wp_scripts global.

---

⚠ I just realized that Image Prioritizer is also referencing the private constants in rest-api.php:

performance/plugins/image-prioritizer/helper.php

Lines 297 to 304 in d21b74d

	if (
	$request->get_method() !== 'POST'
	||
	// The strtolower() and outer trim are due to \WP_REST_Server::match_request_to_handler() using case-insensitive pattern match and using '$' instead of '\z'.
	OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== rtrim( strtolower( ltrim( $request->get_route(), '/' ) ) )
	) {
	return $response;
	}

[felixarntz]

These APIs are all private, yes. Both OD_URL_Metrics_Post_Type and OD_Storage_Lock are private, and all the functions and constants in rest-api.php have @access private as well. So we're free to change these however we see fit.

👍

At this point we could make the OD_URL_Metric_Group_Collection instance available as a global without other extensions needing to reconstruct it. But would we make this a new $od_url_metric_group_collection global? Would we have a od_url_metric_group_collection() function that has a static variable like you said? But being a static variable then you can't clear it out. Compare with wp_scripts() which sets the $wp_scripts global.

Is clearing out the variable relevant? If this is a theoretical concern at this point, I wouldn't worry about it. I wouldn't look at something like wp_scripts() as a reference, since it is an ancient function that was added to Core long before global variables were discouraged. So I think we should go with a function and static variable for this kind of situation. Even with this model, we could somehow expand it later to allow resetting it, e.g. via optional parameter. That's not necessarily a great pattern, but just thinking out loud - it wouldn't block us from making clearing/resetting possible.

Alternatively, if we don't want to introduce a global function, we could introduce an instance() static method on the class itself. Basically like a singleton, but without preventing manual instantiation of the class.

⚠ I just realized that Image Prioritizer is also referencing the private constants in rest-api.php:

Probably fine to remove the private annotation on them. The constants are technically public anyway, and it's fine that we're stuck with them forever. The good thing is that they're constants, so we could potentially change the value while still maintaining backward compatibility, as long as we make it clear that you should always use the constant if you want to refer to the endpoint.

[westonruter]

Is clearing out the variable relevant? If this is a theoretical concern at this point, I wouldn't worry about it.

It's not theoretical as unit tests wouldn't work without it being reset.

[felixarntz]

Is clearing out the variable relevant? If this is a theoretical concern at this point, I wouldn't worry about it.

It's not theoretical as unit tests wouldn't work without it being reset.

Can you share an example? Ideally, most unit tests that test for specific scenarios shouldn't use the main instance anyway, but use a new instance to test the specific scenario. That's what the encapsulation from using instances is supposed to enable in the first place.

Testing the whole functionality using the main instance can also be useful of course, but it's mostly an integration test then and I'd argue for that we should go with whatever the default behavior is, while using separate instances to cover specific scenarios.

[westonruter]

For example, all of the snapshot tests involve passing the entire HTML of the buffer into the od_optimize_template_output_buffer() function which loads the URL Metrics from the post type, constructs the collection, and then iterates over the document.

performance/plugins/optimization-detective/tests/test-optimization.php

Line 386 in d21b74d

public function test_od_optimize_template_output_buffer( string $directory ): void {