Skip to content
This repository has been archived by the owner on Oct 19, 2022. It is now read-only.

Only ramp post types that are supported by Gutenberg #32

Merged
merged 9 commits into from
May 14, 2018
Merged

Only ramp post types that are supported by Gutenberg #32

merged 9 commits into from
May 14, 2018

Conversation

pyronaur
Copy link
Member

This fixes #13 by preventing ramp_for_gutenberg_load_gutenberg from enabling Ramp on post types that can't support Gutenberg (related Gutenberg issue: WordPress/gutenberg#3066 )

Only post_type argument is affected by this PR
Single post_ids can still be enabled in Ramp at the moment, even if they're not supported by Gutenberg. I'm a bit cautious and unsure whether we should check those too, because Ramp will have to run get_post_type( $post_id ) for each post in the post_ids list on every admin page load.

Test plan:

  1. Install a plugin with a custom post type that doesn't want an editor UI. For example: "WP RSS Aggregator"
  2. Make sure WP_DEBUG is enabled
  3. Use the following code to enable ramp:
ramp_for_gutenberg_load_gutenberg( [ 'post_types' => [ 'post', 'wprss_feed' ] ] );
  1. Experience an error message that looks something like this:

screen shot 2018-05-10 at 14 53 35

Notice: ramp_for_gutenberg_load_gutenberg was called <strong>incorrectly</strong>. 
Cannot enable Gutenberg support for post type "wprss_feed" Please see <a href="https://codex.wordpress.org/Debugging_in_WordPress">Debugging in WordPress</a>

@pyronaur pyronaur requested a review from mattoperry May 10, 2018 12:02
Copy link
Contributor

@mattoperry mattoperry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just one question inline.

@@ -45,8 +61,12 @@ public function get_criteria( $criteria_name = '' ) {
}

public function save_criteria( $criteria ) {
if ( $this->validate_criteria( $criteria ) ) {
return update_option( $this->get_option_name(), $criteria );
self::$criteria = $criteria;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justnorris wondering if the validation should happen here rather than before the option is written -- ie: would we want to assign invalid criteria to self::$criteria ?

@pyronaur
Copy link
Member Author

Ok, I made some changes to how this works.

When storing criteria (even if just temporarily) - sanitize the array first, just like we did before. But instead of directly updating it in options, keep the data in a private class variable.

Then, on admin_init - validate the options to values that we expect and if they pass - store them.

Thank you @mattoperry for the review! I think this is a lot better.

@mattoperry mattoperry merged commit b29941c into Automattic:master May 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If a post isn't visible in REST, ramp should not try to load gutenberg for it
2 participants