Add admin screen for managing which post types have AMP support #811
Conversation
| if ( true !== $show_options_menu ) { | ||
| /** | ||
| * Filter whether to enable the AMP settings. | ||
| * |
There was a problem hiding this comment.
Could you find when this filter was introduced and add a @since tag for the version?
includes/amp-helper-functions.php
Outdated
| // Because `add_rewrite_endpoint` doesn't let us target specific post_types :( | ||
| if ( ! post_type_supports( $post->post_type, AMP_QUERY_VAR ) ) { | ||
| // Listen to post types settings. | ||
| $is_enabled_via_setting = (bool) AMP_Settings_Post_Types::get_instance()->get_settings_value( $post->post_type ); |
There was a problem hiding this comment.
Question: Instead of sourcing the enabled state via the settings directly, why not just add a separate action hook that then just calls add_post_type_support() for each post type enabled be admin settings. This would allow plugins to query for AMP post type support via just post_type_supports() calls, and it could allow a plugin to turn off post type support for AMP via just remove_post_type_support(). Then this post_supports_amp() function could be left unchanged, as the plugin would essentially be providing a UI for doing the same add_post_type_support() calls that existing themes/plugins are doing today.
There was a problem hiding this comment.
Good question, that is how I had it initially (see this commit which removed it) but here is the issue I faced. If post_type_supports() is set by a theme/plugin then the admin setting must be checked and disabled according to the AC. The post_type_supports() support is usually declared in a init action which runs before the admin checks so if we add post_type_supports based on the db value saved then the checkbox becomes disabled (unless we declare it in an action running later which doesn't feel right). So I added a flag to differentiate the post_type_supports() that was added by the setting and the one that would be declared by a theme/plugin to prevent this behaviour. The issue is that if one is saved in the db and then a plugin/theme add support for it, then it won't be following the intended behaviour.
I will review this quickly right now and post and update.
There was a problem hiding this comment.
Alright, this commit declare the post_type_supports() based on the admin settings. To do so, it runs it in a after_setup_theme callback so that it can be overwritten by theme/plugins which would declare add_post_type_support() or remove_post_type_support() invoked through an init action.
Regarding the admin option disabled state, since we can't flag a post type which is not enabled by setting and removed by plugin/theme we can't disable the checkbox but the AMP_Settings_Post_Types::errors() takes care of this scenario if a user try to enable a post type support but a theme/plugin declares remove_post_type_support().
One scenario that could be seen as an issue is if a plugin doesn’t have add_post_type_support() in its current version and the AMP settings are saved. If in the newer version the plugin add add_post_type_support() then the checkbox in the admin wouldn’t be disabled since it is flagged that the post type support is managed by the setting is saved in the db. For that reason, I would be in favor of removing the "inconsistent" disabled state and add an error handling in case a plugin/theme calls add_post_type_support() and a user tries to disable it. That would be a complete bullet proof and consistent solution. Thoughts?
There was a problem hiding this comment.
If in the newer version the plugin add add_post_type_support() then the checkbox in the admin wouldn’t be disabled since it is flagged that the post type support is managed by the setting is saved in the db
What if the plugin instead had a constant that defined the post types that the AMP plugin supports by default? This could then be used in the amp_core_post_types_support() function, and then when deciding whether or not to disable the checkbox it can look at the constant rather than (or in addition to?) doing the post_type_supports() call?
There was a problem hiding this comment.
One scenario that could be seen as an issue is if a plugin doesn’t have
add_post_type_support()in its current version and the AMP settings are saved. If in the newer version the plugin addadd_post_type_support()then the checkbox in the admin wouldn’t be disabled since it is flagged that the post type support is managed by the setting is saved in the db.
Plugin wouldn't be able to change the constant unless we add a filter, hence my suggestion to add a filter via PM earlier this week. That said, this would not take care of plugins which would want to declare remove_post_type_support() but have the checkbox disabled which is a pity if we are going to root of adding an extra flag.
If we are going to add an extra step, then I would vote for a filter like amp_post_types_support_checkbox_disabled. That way we can let the error handling taking care of overwrite restrictions on save if the plugin didn't add amp_post_types_support_checkbox_disabled and allow plugins to force disable the checkbox to enhance user experience. Alternatively, we could just not have the disabled state and let error handle all scenarios on save which will be functionality bullet proof and avoid and inconsistent user experience. Thoughts?
There was a problem hiding this comment.
I'm having a hard time running through the scenarios in my head. Can we create a little sample plugin that registers some post types, enables support for some of them, and disables support for others?
There was a problem hiding this comment.
@westonruter following on our discussion at WCUS, I pushed a commit which removed the disabled state and handle all scenarios using an errors handling on save. This covers all type of scenarios in a consistent and bullet proof way.
While it was nice to have the disable state on load from a user experience perspective, I believe the fact that it was so inconsistent (no way around it) didn't justify to have it and actually turned into a bad user experience as no one could really figure out what was going on. Please take it for a spin and let me know what you think.
| '_builtin' => false, | ||
| ), 'objects' ); | ||
|
|
||
| return $core + $cpt; |
There was a problem hiding this comment.
I think array_merge() should be used here. Using + with arrays can have unexpected results if the items are numerically-indexed: https://stackoverflow.com/a/7059731/93579
| */ | ||
|
|
||
| /** | ||
| * Settings post types class. |
There was a problem hiding this comment.
Let's add @since tags to the classes and methods being introduced.
| */ | ||
| public function init() { | ||
| add_action( 'admin_init', array( $this, 'register_settings' ) ); | ||
| add_action( 'update_option_' . AMP_Settings::SETTINGS_KEY, 'flush_rewrite_rules' ); |
| * Register the current page settings. | ||
| */ | ||
| public function register_settings() { | ||
| register_setting( AMP_Settings::SETTINGS_KEY, AMP_Settings::SETTINGS_KEY ); |
There was a problem hiding this comment.
This should add a sanitize_callback here for good measure, even though it is only ever read out with a cast to boolean.
| public static function get_instance() { | ||
| static $instance; | ||
|
|
||
| if ( ! $instance instanceof AMP_Settings_Post_Types ) { |
There was a problem hiding this comment.
Operator precedence here makes me nervous. Can this be changed to:
! ( $instance instanceof AMP_Settings_Post_Types )
| <fieldset> | ||
| <?php foreach ( $this->get_supported_post_types() as $post_type ) : ?> | ||
| <label> | ||
| <input type="checkbox" value="1" name="<?php echo esc_attr( $this->get_setting_name( $post_type->name ) ); ?>"<?php checked( true, (bool) $this->get_settings_value( $post_type->name ) || post_type_supports( $post_type->name, AMP_QUERY_VAR ) ); ?><?php disabled( true, post_type_supports( $post_type->name, AMP_QUERY_VAR ) ); ?>> <?php echo esc_html( $post_type->label ); ?> |
There was a problem hiding this comment.
My comment about is_enabled_via_setting above will impact this. In particular, the disabled() call would need to be changed to:
disabled( true, post_type_supports( $post_type->name, AMP_QUERY_VAR ) && ! $this->get_settings_value( $post_type->name ) )
ThierryA
force-pushed
the
feature/789-cpt-admin-settings
branch
from
09d3685 to
68c5017
Compare
| * | ||
| * @since 0.6 | ||
| */ | ||
| function define_query_var() { |
There was a problem hiding this comment.
We should add hook docs for this filter here since we're touching it.
It was introduced in 0.3.2 via c486a1c
includes/amp-post-types-support.php
Outdated
| } | ||
| } | ||
| } | ||
| add_action( 'after_setup_theme', 'amp_custom_post_types_support' ); |
There was a problem hiding this comment.
I wonder if this shouldn't have a lower priority, like 5.
The _s starter theme and Twenty Seventeen among others do their add_theme_support calls in after_setup_theme at priority 10.
https://github.com/Automattic/_s/blob/efd37d13a6622bcd20e8c7b35309102c06981c71/functions.php#L10-L84
https://github.com/WordPress/wordpress-develop/blob/a825a181e11b1a89e76d79d22bd2c220f45b3741/src/wp-content/themes/twentyseventeen/functions.php#L20-L217
So if they also do calls to add_post_type_support() I'd expect them to do it here as well, for example: https://github.com/ryelle/Anadama-React/blob/21c8f550bc3ea6bbb7407891bf06845316b0ebf6/functions.php#L78-L82
|
@ThierryA My reply to your comment got collapsed since it was part of a patch that had a change: #811 (comment) |
|
@westonruter this PR is good to go unless we decide to make other changes regarding the admin checkbox disable status (see my reply here). Thanks, |
|
I'm going to be pushing an update here to merge the new methods into the existing menu and options manager classes. |
|
Unit tests need to be updated but I think 1befc79 will get us better set for future migration to Customizer while starting to unify option handling. |
| id="<?php echo esc_attr( $id ); ?>" | ||
| name="<?php echo esc_attr( $id ); ?>" | ||
| <?php checked( true, post_type_supports( $post_type->name, AMP_QUERY_VAR ) ); ?> | ||
| <?php disabled( $is_builtin ); ?> |
There was a problem hiding this comment.
@ThierryA This is what I was talking about in regards to hard-coding a set of post types that are always active, and thus are disabled in the UI. Maybe we could instead just hide them altogether, but it turns out to be useful to have them present. The hidden input above ensures that there are some checked checkboxes which means we will get a non-empty array passed in to validate.
There was a problem hiding this comment.
Thanks @westonruter. That makes perfect sense in a situation where only built-in post types would get the disabled state rather than all post types which enable/disable support via the code like the AC scope. I am in full agreement with the solution proposed here as we have already discussed this matter inside out and clearly identified the issues occurring when trying to apply the disabled state according to the original AC.
|
@ThierryA ok, this is ready for your final review and merge! |
westonruter
changed the title
There was a problem hiding this comment.
Hi @westonruter, below is the CR for the latest changes.
PS: I can not change the status to "Request changes" since GitHub doesn't allow to "self-review" a PR and this PR was originally open by me.
amp.php
Outdated
| do_action( 'amp_init' ); | ||
| add_action( 'admin_init', 'AMP_Options_Manager::register_settings' ); |
There was a problem hiding this comment.
Wouldn't it make sense to move that to the includes/admin/functions.php file since all the other admin classes are booted in there?
| } | ||
|
|
||
| /** | ||
| * Declare custom post types support. |
There was a problem hiding this comment.
This function is also add support of built-in post types.
| public static function add_post_type_support() { | ||
| $post_types = array_merge( | ||
| self::get_builtin_supported_post_types(), | ||
| array_keys( array_filter( AMP_Options_Manager::get_option( 'supported_post_types', array() ) ) ) |
There was a problem hiding this comment.
What is the array_filter() used for since only enabled post types are saved in the db?
There was a problem hiding this comment.
True. Eventually I think we should just store an array of post types rather than an array mapping post type names to true. In fact, there's no reason why we can't do that now.
| */ | ||
| public static function validate_options( $new_options ) { | ||
| $defaults = array( | ||
| 'supported_post_types' => array(), |
There was a problem hiding this comment.
I think it would be beneficial to store the post type and analytics option names as constants, like the AMP OPTION_NAME is stored.
There was a problem hiding this comment.
Yes, that would be an improvement. Not a blocker though.
|
|
||
| // Validate post type support. | ||
| if ( isset( $new_options['supported_post_types'] ) ) { | ||
| $options['supported_post_types'] = array(); |
There was a problem hiding this comment.
Any specific reason to re-declare the array here since it is already done on line 74 and can never be saved in the db as another format?
There was a problem hiding this comment.
On 74 it is to define it if it is not defined, as a default. Here we explicitly reset the previously saved value before gathering the new post types.
| 'config' => $entry_config, | ||
| ); | ||
| } | ||
| // Redirect to keep the user in the analytics options page. |
There was a problem hiding this comment.
Minor (doesn't need to be changed): this format doesn't follow WP multi-line coding standards (see here).
| } | ||
| // Redirect to keep the user in the analytics options page. | ||
| // Wrap in is_admin() to enable phpunit tests to exercise this code. | ||
| wp_safe_redirect( admin_url( 'admin.php?page=amp-analytics-options&settings-updated=1' ) ); |
There was a problem hiding this comment.
I would be better not to have the url hard coded.
There was a problem hiding this comment.
Yes, but this is how it is currently in the released version of the plugin: https://github.com/Automattic/amp-wp/blob/6df01e9c90f2de3c4cb16952401a285992d11021/includes/options/views/class-amp-options-manager.php#L42
I didn't want to spend more time on improving analytics settings in the admin since we want to move them to the Customizer.
| ) ); | ||
| $entries = AMP_Options_Manager::get_option( 'analytics' ); | ||
| $this->assertCount( 1, $entries ); | ||
| $this->assertArrayNotHasKey( $id, $entries ); |
There was a problem hiding this comment.
Would be good to cleanup the option here not to leave traces behind which might affect other tests.
There was a problem hiding this comment.
Unnecessary because tests in core automatically clean up after themselves: https://github.com/WordPress/wordpress-develop/blob/1c2b0762c7f1a91394c633ae812505919bd297ed/tests/phpunit/includes/testcase.php#L102-L182
| * @covers AMP_Post_Type_Support::get_eligible_post_types() | ||
| */ | ||
| public function test_get_eligible_post_types() { | ||
| register_post_type( 'book', array( |
There was a problem hiding this comment.
These post types are not removed on tear down which could affect other tests.
There was a problem hiding this comment.
Unnecessary because tests in core automatically clean up after themselves: https://github.com/WordPress/wordpress-develop/blob/1c2b0762c7f1a91394c633ae812505919bd297ed/tests/phpunit/includes/testcase.php#L102-L182
There was a problem hiding this comment.
I might be missing something but I don't see where the registered post types are removed, for instance if remove the post type registration in test_add_post_type_support() the tests still pass.
There was a problem hiding this comment.
I ended up explicitly unregistering the post types in 8c14063 because it seems core isn't fully cleaning up after itself as I thought.
| * @covers AMP_Post_Type_Support::add_post_type_support() | ||
| */ | ||
| public function test_add_post_type_support() { | ||
| register_post_type( 'book', array( |
There was a problem hiding this comment.
These post types are not removed on tear down which could affect other tests.
There was a problem hiding this comment.
Unnecessary because tests in core automatically clean up after themselves: https://github.com/WordPress/wordpress-develop/blob/1c2b0762c7f1a91394c633ae812505919bd297ed/tests/phpunit/includes/testcase.php#L102-L182
|
@ThierryA there is nothing left outstanding here I believe. |
|
Thanks @westonruter, looking great. This is good to go! |
This PR add a "AMP -> Settings" submenu above the "Analytics" submenu (some refactoring was needed to achieve that). The new Settings page has the Post Types support settings allowing users to enable/disable AMP for custom post types. The "post" post type is always on as per the ACs.
Refer to the ACs for more info...
Close #798