Comments (8)
๐ 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.
Related Issues (20)
- In settings, improve checkbox UI for enabling custom post types HOT 6
- Cross linking an issue in regards to additional editing options HOT 1
- Suggested enhancements - to plugin or to documentation HOT 4
- Disable 'try Gutenberg' banner in wp 4.9.8, if using Ramp HOT 7
- Admin Alert when Gutenberg is off - dismissible forever HOT 1
- Autoloading ramp options HOT 3
- Does not support 'fallback' mode HOT 2
- allow opt-in to Gutenberg by post/page slug or tag HOT 2
- Bug: Gist embedding not displaying in editor HOT 1
- wpcom_vip_load_gutenberg doesn't allow meta boxes to be loaded HOT 3
- Disable Ramp when Gutenberg version requirements aren't met HOT 3
- All options in the UI should be greyed out in the cases where Ramp sets Gutenberg to always or never load
- Make Ramp + Classic Editor play nicer together HOT 2
- Make it clearer how to enable Gutenberg for all post types HOT 1
- Add some clarity to the documentation about Gutenberg requirement HOT 1
- "Greater Than" HOT 4
- Ability to modify admin screen access
- The UI isn't displaying at all. HOT 1
- Plugin always loads classic-editor when classic-editor plugin is installed HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ๐๐๐
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google โค๏ธ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from gutenberg-ramp.