Pass WP_Query to Content Getter Implementations.

[hellofromtonya]

This PR improves the reuse of the plugin by passing query to the content getter implementation. An implementation may need more information than just the quantity to process and return. By passing the query to the implementation, it can handle its own work.

This PR also improves the testing suite for the filtering process.

This PR also fixes the plugin's bootstrap so that the plugin does actually filter the query when doing a RESTful request. Booyah! #

[hellofromtonya]

@Shelob9 After I created this PR, it dawned on me focus this PR on just passing the query around. The plugin does load now and handles filtering the query.

I think it's better to have the Factory and Args Modifier enhancements in another branch and PR. Don't you? It's easier to track what we're doing and deal with any issues that might arise.

[Shelob9]

Yes. I agree.

But I don't think this refactor brings us to a place where we can have one factory that wires everything.

My concerns to get there:

We still have ModifySchema and ModifyQueryArgs that change the endpoint's schema and the allowed WP_Query args. Those don't have an interface. They all have a shouldFilter method, but they have different signatures. FiltersPreWPQuery also has a shouldFilter different signature, also static. This smells bad.

Maybe, the ContentGetterContract should require the WP_Query and the WP_REST_Request. The point of this is REST API searches, right now. Points for encapsulation and black boxing the entire HTTP API from the end implementation, but also, what if I want to read the headers, or the raw request data in my implementation? What if my implementation doesn't use WP_Query and already has a good system for converting WP_Rest_Requests to my own entity/model/etc ?

[Shelob9]

@hellofromtonya I did phpcs:disable here because I didn't know why setup_common_wp_stubs isn't camel cased.

[hellofromtonya]

Make it camelCase. It's a copy from my other work, which uses WPCS 100%.

[Shelob9]

I really do think, if I'm designing an implementation I want a WP_REST_Request here.

[hellofromtonya]

Okay, then let's make that change.

[Shelob9]

Ok, but you asked for upsides and downsides so I wrote the last paragraph of this comment: #6 (comment) and now I really feel like the change either:

• Strongly couples the interface to the REST API. 👎
  or
• We have to have another level of abstraction below it. Ok. I'll do that I guess.

[Shelob9]

Change made
e2e1719

[Shelob9]

Maybe a separate concern, but this interface will need to communicate total in the current result set and total pages.

[hellofromtonya]

Remember here that our PostsGenerator is an experimental implementation to merely mock creating posts. It has a private function to get how many posts to create.

In a real-world application, you would want a different approach as you've indicated. But here, this approach fits the needs of the educational experiment.

[Shelob9]

Right.

[Shelob9]

BTW @hellofromtonya I made ba0f8e5 on this branch because it's just the results of the code sniffer/ fixer. I am branching from here to work on creating the one factory like we discussed. Just wanted to make merge easier.

[hellofromtonya]

We still have ModifySchema and ModifyQueryArgs that change the endpoint's schema and the allowed WP_Query args. Those don't have an interface. They all have a shouldFilter method, but they have different signatures. FiltersPreWPQuery also has a shouldFilter different signature, also static. This smells bad.

If we were to use the exact same interface design for methods like shouldFilter, then this would be true. However, in this plugin, FiltersPreWPQuery is static by design. Therefore it's signature is different as well as its implementation.

It's okay to have different signatures and implementations if that's the intent. It's not a code smell. Why? We've intentionally designed this method for this specific purpose.

Now, if your intent is to design a global pattern and signature whereby all of the implementations have a shouldFIlter and that method has the same intent and signature, albeit different code to determine whether to filter or not, then that's a different design.

The key then is to determine:

1. What is the intent?
2. Do you want to force the shouldFilter pattern on all of the filtering interfaces?
3. Would you potentially box yourself into a corner?
4. What advantage do you get from the direction you want to go?

[Shelob9]

Gutenberg hooks into both of the filters we need to work with rest_{$post_type}_collection_params and rest_{$post_type}_query here:

https://github.com/WordPress/gutenberg/blob/8c708bc846077926e49da052cdde119c02abad7a/lib/rest-api.php#L333

/**
 * Whenever a post type is registered, ensure we're hooked into it's WP REST API response.
 *
 * @param string $post_type The newly registered post type.
 * @return string That same post type.
 */
function gutenberg_register_post_prepare_functions( $post_type ) {
	add_filter( "rest_prepare_{$post_type}", 'gutenberg_add_permalink_template_to_posts', 10, 3 );
	add_filter( "rest_prepare_{$post_type}", 'gutenberg_add_block_format_to_post_content', 10, 3 );
	add_filter( "rest_prepare_{$post_type}", 'gutenberg_add_target_schema_to_links', 10, 3 );
	add_filter( "rest_{$post_type}_collection_params", 'gutenberg_filter_post_collection_parameters', 10, 2 );
	add_filter( "rest_{$post_type}_query", 'gutenberg_filter_post_query_arguments', 10, 2 );
	return $post_type;
}
add_filter( 'registered_post_type', 'gutenberg_register_post_prepare_functions' );

[Shelob9]

OK, so I'm as far into #7 as creating interfaces for the two other classes.

Intent: we want queries to post type routes such as/wp/v2/posts to be more useful for search on some post types. By more useful we mean different types of WP_Querys or results generated with a different system.

So I think to wire an arbitrary number of implementations, we need to hook rest_{$post_type}collection_params and rest{$post_type}_query for every post type. Then we have some sort of container that is aware of the WP_REST_Request and if FiltersPreWPQuery says that a rest request is happening all registered sets of the three objects can be set correctly and supplied the right dependencies.

So, I think we want to make the filtering as magic as possible, since that's repetitive. End implementation should supply the new query args and new schema args and a class that implements ContentGetterContract.

I think making implementations will be more flexible if ContentGetterContract::getContent() has a WP_Rest_Request, but I also think that means the content generator can't work for any requirement that isn't a REST API request.

[Shelob9]

Now with 3268a62 I have a factory that takes an array fo arguments for the REST API to add, and an array of post types -- like this that auto-wires those filters.

BTW a lot of my concern about setting objects went away by letting the plugins API supply the data.

BTW[] shouldFilter now has a consistent shape between both interfaces :)

Next: an extensible way to load these or route the right ContentGetter

[Shelob9]

With 1bfce07 I'm capturing the current request so it can be injected to content getter using the rest_pre_serve_request filter

[Shelob9]

@hellofromtonya I changed your init() method to being a setter with a corresponding getter which should make last step possible.

Theory is that now getPosts() acts as a dispatcher the last set contentGetter wins. I do wish this router was more abstracted, for example, different implementations per post type.

[Shelob9]

With last commit I think I'm doing today, instantiation of our default implementation uses part of new factory We should now be able to remove ModifyQueryArgs and ModifySchema and also create an add-on. I'll try tomorrow.

[hellofromtonya]

I do wish this router was more abstracted, for example, different implementations per post type.

The design we have now is simplistic, as it wires one implementation to the filter. Then the implementation itself figures out what needs to be done. We've abstracted the tasks of processing getting content from the task of filtering.

In this experiment, it works. But there are cases where one might want to convert getPosts() into a router and then have the ability to switch dynamically between implementations. But that's a different design.

Keep it simple.

I think we can keep it simple for the purposes of this experiment and teaching opportunity. But you can note in your articles to invite devs to expand upon the design for their needs.

[Shelob9]

there are cases where one might want to convert getPosts() into a router and then have the ability to switch dynamically between implementations.
We have that case, I think. We need to be able to change "search mode". This is what I'm doing in #8

In 7c94c74 I added a filter in getPosts so it could be used as a router.

In a4ea96c I introduced a Modes class. @hellofromtonya we should discuss why that name is not semantic enough and what it should be. It takes the responsibility of getting the right Content Generator out. In the first commit its just adding complication to reutrn the same default, but we also have the start of tests to cover this feature

In 82db8a7 I added a filter that if it is null, uses this internal router. If its a valid object, that is returned. I proved this with an integration test. I used an integration test over a unit test for this because it demonstrates how a key part of an add-on implementation would work.

Concerns:

• This is two complex.

• Are two hooks needed? Yes, I think it helps keep this modular.
• Do I need all of this extra complication after the filter? I don't think so.
• Modes is not a semantic enough name.