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

Add admin screen for managing which post types have AMP support #811

Merged
merged 21 commits into from Dec 6, 2017

Conversation

Projects
None yet
2 participants
@ThierryA
Copy link
Collaborator

ThierryA commented Nov 23, 2017

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

  • Update readme to indicate code no longer required.
  • Update unit tests.

Close #798

ThierryA added some commits Nov 23, 2017

@ThierryA ThierryA requested review from westonruter and amedina Nov 23, 2017

if ( true !== $show_options_menu ) {
/**
* Filter whether to enable the AMP settings.
*

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 23, 2017

Member

Could you find when this filter was introduced and add a @since tag for the version?

// 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 );

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 23, 2017

Member

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.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Nov 23, 2017

Author Collaborator

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.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Nov 24, 2017

Author Collaborator

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?

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

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?

This comment has been minimized.

Copy link
@ThierryA

ThierryA Nov 28, 2017

Author Collaborator

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.

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?

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 28, 2017

Member

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?

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 3, 2017

Author Collaborator

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

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 23, 2017

Member

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.

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 23, 2017

Member

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' );

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 23, 2017

Member

Good you thought of this.

* Register the current page settings.
*/
public function register_settings() {
register_setting( AMP_Settings::SETTINGS_KEY, AMP_Settings::SETTINGS_KEY );

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 23, 2017

Member

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 ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 23, 2017

Member

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 ); ?>

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 23, 2017

Member

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 ThierryA force-pushed the feature/789-cpt-admin-settings branch from 09d3685 to 68c5017 Nov 24, 2017

@ThierryA ThierryA changed the base branch from master to develop Nov 24, 2017

*
* @since 0.6
*/
function define_query_var() {

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

We should add hook docs for this filter here since we're touching it.

It was introduced in 0.3.2 via c486a1c

}
}
}
add_action( 'after_setup_theme', 'amp_custom_post_types_support' );

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

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

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Nov 27, 2017

@ThierryA My reply to your comment got collapsed since it was part of a patch that had a change: #811 (comment)

ThierryA added some commits Nov 28, 2017

@ThierryA

This comment has been minimized.

Copy link
Collaborator Author

ThierryA commented Dec 3, 2017

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

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Dec 5, 2017

I'm going to be pushing an update here to merge the new methods into the existing menu and options manager classes.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Dec 5, 2017

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 ); ?>

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 5, 2017

Member

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

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

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.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Dec 6, 2017

@ThierryA ok, this is ready for your final review and merge!

@westonruter westonruter changed the title #789 Post type admin settings Add admin screen for managing which post types have AMP support Dec 6, 2017

@ThierryA
Copy link
Collaborator Author

ThierryA left a 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' );

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

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.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

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() ) ) )

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

What is the array_filter() used for since only enabled post types are saved in the db?

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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(),

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

I think it would be beneficial to store the post type and analytics option names as constants, like the AMP OPTION_NAME is stored.

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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();

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

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?

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

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' ) );

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

I would be better not to have the url hard coded.

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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 );

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

Would be good to cleanup the option here not to leave traces behind which might affect other tests.

This comment has been minimized.

* @covers AMP_Post_Type_Support::get_eligible_post_types()
*/
public function test_get_eligible_post_types() {
register_post_type( 'book', array(

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

These post types are not removed on tear down which could affect other tests.

This comment has been minimized.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

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.

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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(

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 6, 2017

Author Collaborator

These post types are not removed on tear down which could affect other tests.

This comment has been minimized.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Dec 6, 2017

@ThierryA there is nothing left outstanding here I believe.

@ThierryA

This comment has been minimized.

Copy link
Collaborator Author

ThierryA commented Dec 6, 2017

Thanks @westonruter, looking great. This is good to go!

@westonruter westonruter merged commit c73f479 into develop Dec 6, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the feature/789-cpt-admin-settings branch Dec 6, 2017

@westonruter westonruter added this to To Do in v0.6 via automation Dec 6, 2017

@ThierryA ThierryA removed this from To Do in v0.6 Dec 7, 2017

@ThierryA ThierryA added this to To Do in v0.6 via automation Dec 7, 2017

@ThierryA ThierryA removed this from To Do in v0.6 Dec 7, 2017

@westonruter westonruter added this to the v0.6 milestone Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.