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.

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

Pass WP_Query to Content Getter Implementations. #6

Open
wants to merge 10 commits into
base: master
from

Conversation

@hellofromtonya
Copy link
Collaborator

commented May 27, 2018

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! #

An implementation might need more information that's available in the
query.  Therefore, instead of passing a query to go get, let's pass the
query and let each implementation initialize itself.

This enhancement makes the new implementation design even more reusable.
1. Fixed the RestRequestTest.
2. Modified the testGetPostsArePostsShouldFilter() test to focus it on
testing when a query is supplied.
3. Added e2e test for FilterWPQuery by running a REST request.
1. Load Patchwork early, before everything else.
2. Don't load the plugin's bootstrap file as part of the unit test startup process.
3. Moved the internationalization mocking to the Test Case. Leveraged the stubs
in Brain Monkey to handle it for us.
@hellofromtonya hellofromtonya changed the title Glues everything together Pass WP_Query to May 27, 2018
@hellofromtonya hellofromtonya changed the title Pass WP_Query to Pass WP_Query to Content Getter Implementations. May 27, 2018
@hellofromtonya hellofromtonya requested a review from Shelob9 May 27, 2018
@hellofromtonya

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2018

@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.

hellofromtonya and others added 3 commits May 27, 2018
You know that moment when you look at your code and think "What the heck was I thinking?!"
This commit fixes one of _those_ moments.

In the integration tests, pass the number of posts into WP_Query as an argument. Duh.
@Shelob9

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

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 ?

/**
* Set up the stubs for the common WordPress escaping and internationalization functions.
*/
protected function setup_common_wp_stubs()

This comment has been minimized.

Copy link
@Shelob9

Shelob9 May 28, 2018

Contributor

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

This comment has been minimized.

Copy link
@hellofromtonya

hellofromtonya May 28, 2018

Author Collaborator

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

*
* @return array
*/
public function getContent(WP_Query $query): array

This comment has been minimized.

Copy link
@Shelob9

Shelob9 May 28, 2018

Contributor

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

This comment has been minimized.

Copy link
@hellofromtonya

hellofromtonya May 28, 2018

Author Collaborator

Okay, then let's make that change.

This comment has been minimized.

Copy link
@Shelob9

Shelob9 May 28, 2018

Contributor

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.

This comment has been minimized.

Copy link
@Shelob9

Shelob9 May 28, 2018

Contributor

Change made
e2e1719

*/
private function getQuantity(): int
{
if (!isset($this->query->query)) {

This comment has been minimized.

Copy link
@Shelob9

Shelob9 May 28, 2018

Contributor

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

This comment has been minimized.

Copy link
@hellofromtonya

hellofromtonya May 28, 2018

Author Collaborator

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.

This comment has been minimized.

Copy link
@Shelob9

Shelob9 May 28, 2018

Contributor

Right.

@Shelob9

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2018

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

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

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

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

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

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

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

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

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

@Shelob9

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2018

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 Shelob9 referenced this pull request May 29, 2018
@Shelob9

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.