generalize criteria for GB loading about gutenberg-ramp HOT 8 OPEN

👋 I have 2¢ to add.

I agree that making Ramp more flexible would be a good thing, however, I'm not sure that using a new WP_Query is the way to do it.

Consider this query:

			$args = [
				'post_id'       => get_the_ID(),
				'category_name' => 'uncategorized',
				'date_query'    =>
					[
						'after' => 'August 1st, 2013',
					],
		
				'meta_query' => [
					[
						'key'     => 'special_post',
						'compare' => 'NOT EXISTS',
					],
				],
			];
		
			$query = new WP_Query( $args );

It produces the following SQL Queury:

SELECT SQL_CALC_FOUND_ROWS wp_6_posts.ID
FROM wp_6_posts 
LEFT JOIN wp_6_term_relationships
ON (wp_6_posts.ID = wp_6_term_relationships.object_id)
LEFT JOIN wp_6_postmeta
ON (wp_6_posts.ID = wp_6_postmeta.post_id
AND wp_6_postmeta.meta_key = 'special_post' )
WHERE 1=1 
AND ( wp_6_posts.post_date > '2012-08-01 00:00:00' )
AND ( wp_6_term_relationships.term_taxonomy_id IN (1) )
AND ( wp_6_postmeta.post_id IS NULL )
AND wp_6_posts.post_type = 'post'
AND (wp_6_posts.post_status = 'publish'
OR wp_6_posts.post_status = 'private')
GROUP BY wp_6_posts.ID
ORDER BY wp_6_posts.post_date DESC
LIMIT 0, 10

I see several problems with the query above:

1. Performance

This is going to show up in the debug bar as a slow query because it takes 0.0889s to perform. And that query is going to be run for every edit.php page. It's not that bad because it's isolated to the admin edit pages, but it would still be great if we could avoid this sort of thing. However, if there were another already existing performance problem on a large-scale site, this would just contribute to the headache.

2. Logic

It's not immediately clear whether or not a post has enabled Gutenberg - from a glance at a post in the dashboard, or even at the $args themselves. The information is scattered and that's going to make it more difficult to parse the information and debug if/when problems arise.

3. Flexibility

While this approach is extremely flexible in the way Ramp would decide whether or not to enable Gutenberg, it would really lack flexibility when it comes to scaling or adding more functionality to the UI, etc. The WP_Query approach would hoard the knowledge in that only running a special query ( $args ) would be necessary to find out whether or not a post should enable Gutenberg.

For example - consider adding a hypothetical "status" indicator that would display whether or not Ramp is active on a each of the posts visible. That would mean that we'd have to either run the slow query above 20 times (for each post).

The problem that I see here is that the information isn't shared in a scalable way with other plugins/tools that may want to use Ramp.

An even simpler example

Even if we vastly simplify the WP_Query to something like this:

			$args = [
				'post_id'       => get_the_ID(),
				'category__in' => [2, 3, 4, 5, 6, 7],
			];
		
			$query = new WP_Query( $args );

It's going to result in 2 queries that take up a total of 0.0027s, which is a lot faster than before, but is still an extra query on each post.

SELECT t.*, tt.*
FROM wp_6_terms AS t 
INNER JOIN wp_6_term_taxonomy AS tt
ON t.term_id = tt.term_id
WHERE tt.taxonomy IN ('category')
AND t.term_id IN ( 2,3,4,5,6,7 )	



SELECT SQL_CALC_FOUND_ROWS wp_6_posts.ID
FROM wp_6_posts 
LEFT JOIN wp_6_term_relationships
ON (wp_6_posts.ID = wp_6_term_relationships.object_id)
WHERE 1=1 
AND ( wp_6_term_relationships.term_taxonomy_id IN (2,3,4,5,6,7) )
AND wp_6_posts.post_type = 'post'
AND (wp_6_posts.post_status = 'publish'
OR wp_6_posts.post_status = 'private')
GROUP BY wp_6_posts.ID
ORDER BY wp_6_posts.post_date DESC
LIMIT 0, 10

Enter Post Meta:

We could use a post meta, such as _ramp_enable_gutenberg to signify that a post should enable Gutenberg.

1. Performance

The best part - WordPress fetches and caches post meta on every post anyway:

SELECT post_id, meta_key, meta_value
FROM wp_6_postmeta
WHERE post_id IN (12338)
ORDER BY meta_id ASC

This is going to have a much higher cache hit ratio. On top of that, it's data we already have so there is no need to add another query to the load.

2. Logic

It's a lot simpler to figure out whether or not Ramp is supposed to load Gutenberg in any post - just check the meta.

3. Flexibility

I think that get_post_meta is infinitely flexible, and we can get the info about the post form anywhere, be it a CLI command, a plugin that extends Gutenberg, or anything else that has access to the function.

Furthermore, consider the date based example below:

Scenario: Date based decision

Purpose: Enable Gutenberg on all new posts created after June 15th, 2015

WP_Query

			gutenberg_ramp_load_gutenberg([
				'query' => [
					'date_query'    =>
						[
							'after' => 'June 15, 2018',
						],
				]
			]);

WP_Query is going to produce this query:

SELECT SQL_CALC_FOUND_ROWS wp_6_posts.ID
FROM wp_6_posts 
WHERE 1=1 
AND ( wp_6_posts.post_date > '2018-06-15 00:00:00' )
AND wp_6_posts.post_type = 'post'
AND (wp_6_posts.post_status = 'publish'
OR wp_6_posts.post_status = 'private') 
ORDER BY wp_6_posts.post_date DESC
LIMIT 0, 10

Then it's going to compare the results and make the decision. Contrast that with this:

get_post_meta approach

The developer would write their own callback for each post that would be automagically called by Ramp to figure out whether or not a post should be loaded.

			gutenberg_ramp_dynamic_load(function( $post_id ) {
				
				$my_timestamp = 1490572800; // March 27, 2018
				
				// Time based query
				if( get_the_time('U', $post_id ) > $my_timestamp ) {
					return true;
				}
				
				// No matches = no action
				return null;

			});

In the background, the gutenberg_ramp_dynamic_load function would do something like this:

			function gutenberg_ramp_dynamic_load( $func ) {

				$post_id = Ramp::get_the_post_id();
				
				// Completely pseudo code, not really important for this example:
				// No post id = creating a new post. Return true by default.
				if( ! $post_id ) {
					return true;
				}


				// Do nothing because Gutenberg is already enabled for this post
				if( false !== metadata_exists( 'post', $post_id, '_ramp_enable_gutenberg') ) {
					return true;
				}

				if( true === call_user_func( $func, $post_id ) ) {
					update_post_meta($post_id, '_ramp_enable_gutenberg', true);
				}

			}

The post meta approach could also allow for this (but I think this needs some more thought):

			gutenberg_ramp_dynamic_load(function( $post_id ) {
				
				// This approach also allows the user to do this:
				if( my_magical_plugin_is_enabled_on_this_post( $post_id ) ) {
					return false;
				}
				
				// No matches = no action
				return null;

			});

-- fin --

Just my 2 cents 🤦‍♂️ 🤣 - I think I went a bit bananas on this. I hope someone has the patience to actually read all that 😃

from gutenberg-ramp.

Hi @mattoperry

Still need to do some performance testing, but I've got a rough sketch of this going:

master...no-sws:feature/wp-query-criteria

Also, I have not looked into post meta caching yet (maybe if performance difference is negligible, it won't be necessary).

Basically, I've just treated the $criteria array as a superset of the WP_Query $args constructor param. I'm not sure I love this and maybe it would be better/more explicit to define another "valid" criteria key (e.g., $criteria['args']).

To load check, I merge the current admin post ID into the post__in WP_Query param (alongside any other user-specified params). Then I check the result set (only retrieving IDs) against the current post ID (just in case the p WP_Query param is used). I also do some preliminary checks to make sure I don't override user-specified post__in and post__not_in WP_Query params.

The only other changes were removing the criteria whitelist check (since the WP_Query params should be available at the top-level). Additionally, I added an empty criteria check as it seemed like the UI post types setting was taking precedence over the helper function criteria. I also allowed for strings to be passed into the gutenberg_ramp_load_gutenberg helper since WP_Query constructor can take a string; this value is converted to an array via wp_parse_args.

So to summarize:

• What do you think of WP_Query params being "merged" to the top-level? Doesn't seem to cause any conflicts w/ existing criteria; however, kinda feels a little messy.
• Any other feedback...

Thanks!

from gutenberg-ramp.

What do you think of WP_Query params being "merged" to the top-level? Doesn't seem to cause any conflicts w/ existing criteria; however, kinda feels a little messy.

It might be okay to just intersperse the WP_Query criteria with everything else .. I'm wondering if it might be even better however to use a separate key for that ... eg something like query that . -- if present -- will contain the criteria and override the other criteria.

I guess one challenge is that we're collecting kind of a motley crew of criteria types. There are the original criteria keys -- post_ids and post_types, there are the boolean criteria true and false and then there would be the query criteria. We'd have to establish an order of precedence in case more than one type of criteria was passed -- which I imagine would be boolean > query > everything else ... how does that sound? ie: it would be a bit difficult to figure out all of the interactions between the different types of criteria so I always imagined that if you use a "higher precedence" kind, then that trumps the lower kind, which becomes ignored even if passed. Thoughts?

from gutenberg-ramp.

@mattoperry That all sounded reasonable to me. I've updated my branch to use the query criteria key (instead of "merged" together at the top-level) (i.e., $criteria['query']). I've also "promoted" the query criteria to follow the boolean in precedence:

master...no-sws:feature/wp-query-criteria

Let me know what you think. Thanks!

from gutenberg-ramp.

@justnorris that sounds like a decent idea to me but I'll defer to @mattoperry .

A couple things I was thinking about:

• Similar to what @mattoperry suggested re: the WP_Query implementation, why not just add this as a callable key (e.g., $criteria['meta']) and keep the main theme helper function?
• I may have missed this in your comment, but how you a user go about updating the "cached" post_meta status? E.g., if they change the callable to contain different logic, would we "clean up" the stashed post meta values for _ramp_enable_gutenberg?

Thanks for the feedback and the performance metrics!

from gutenberg-ramp.

Thanks for the feedback!

I'll defer to @mattoperry .

Yep, I'm keen on hearing his thoughts on this too :)

Similar to what @mattoperry suggested re: the WP_Query implementation, why not just add this as a callable key (e.g., $criteria['meta']) and keep the main theme helper function?

Can you elaborate a bit on that?

I may have missed this in your comment, but how you a user go about updating the "cached" post_meta status? E.g., if they change the callable to contain different logic, would we "clean up" the stashed post meta values for _ramp_enable_gutenberg?

That's a good question! I imagine that once you have Gutenberg enabled for a post, you don't want to disable Gutenberg anymore? However, in case someone does want to, I see 2 possible solutions for that:

• Don't cache the value - update_post_meta smart in that it will trigger an update only if the new value doesn't match the old value. And because we know the post date for each loaded post anyway - it's not that bad to run that piece of code on each admin page anyway.

• Add another helper to clean the Gutenberg status (something like gutenberg_ramp_dynamic_cleanup()? ) and allow the developers to clean the status with the same callback.

from gutenberg-ramp.

Re: $criteria['meta'], I was thinking of something along the lines of:

diff --git a/inc/class-gutenberg-ramp.php b/inc/class-gutenberg-ramp.php
index 7460543..97aa990 100644
--- a/inc/class-gutenberg-ramp.php
+++ b/inc/class-gutenberg-ramp.php
@@ -129,6 +129,12 @@ class Gutenberg_Ramp {
 		// 2. there's an available post_id in the URL to check
 		$gutenberg_ramp_post_id = $this->get_current_post_id();
 
+		if ( ! empty( $criteria['meta'] ) && is_callable( $criteria['meta'] ) ) {
+			$meta_handler = $criteria['meta'];
+
+                     $should_load = $meta_handler( $gutenberg_ramp_post_id );
+
+                     // Cache in meta ...
+
+			return $should_load;
+		}
+
 		// Check if query criteria available
 		if ( ! empty( $criteria['query'] ) ) {
 			$args = $criteria['query'];

Note I haven't tested this (wondering if WP can serialize a callable value), but basic idea is to just use a meta handler at some level of precedence that receives the current post ID.

At this point, I'm wondering if it'd be more useful/flexible to just provide a filter the end user could hook into...

Thanks!

from gutenberg-ramp.

This sounds really powerful and useful – is there a PR in progress for this is issue? When do you think we might have a fix?

In WooCommerce.com we'd really appreciate a facility like this :)

from gutenberg-ramp.