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

Changing settings can result in erroneous notices regarding post types #1302

Closed
westonruter opened this Issue Aug 1, 2018 · 5 comments

Comments

3 participants
@westonruter
Copy link
Member

westonruter commented Aug 1, 2018

I've found sometimes when switching between template modes or supported templates that I get blasted with error notices regarding post type support, even when I made no changes to the post types:

image

@westonruter westonruter added this to the v1.0 milestone Aug 1, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 5, 2018

I noticed this again when switching the template mode from Classic to Native/Paired after upgrading a site from 0.7 to 1.0-beta2.

@westonruter westonruter added this to To do in v1.0 Aug 5, 2018

@hellofromtonya hellofromtonya moved this from To do to In progress in v1.0 Aug 9, 2018

@hellofromtonya hellofromtonya self-assigned this Aug 9, 2018

@hellofromtonya

This comment has been minimized.

Copy link
Contributor

hellofromtonya commented Aug 10, 2018

I've found sometimes when switching between template modes or supported templates that I get blasted with error notices regarding post type support, even when I made no changes to the post types:

@westonruter Did you happen to notice which post types were selected? Was it just the default of Posts?

I noticed this again when switching the template mode from Classic to Native/Paired after upgrading a site from 0.7 to 1.0-beta2.

I was able to reproduce the notice blasting conditions when upgrading from 0.7 to 1.0-beta2 but only when the default post type of 'Posts` was selected (none of the others). Then upon upgrading and switching from Classic to Paired or Native, the notice blasts occurred.

Click on this GIF to watch the steps I went through to reproduce the problem upon upgrade:

amp-issue-1302

Why?

When first switching from Classic to Paired or Native, notice that the Serve all templates as AMP regardless of what is being queried. , i.e. option amp-options[all_templates_supported], is on (checked) by default.

We should revisit what state we want that all_templates_supported option to be when first upgraded or installed.

  • If first installed, then it can be set to on.
  • Else, set it to off as the default and use the post types that were previously set in the last version of the plugin.
@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 10, 2018

Did you happen to notice which post types were selected? Was it just the default of Posts?

Yes, just Posts was selected, and forcibly so since the checkbox is disabled.

We should revisit what state we want that all_templates_supported option to be when first upgraded or installed.

Yes, the two options you've outlined seem perfect. If all post types were not selected when upgrading to 1.0, then all_templates_supported should be initially false.

hellofromtonya added a commit that referenced this issue Aug 14, 2018

Initialize `all_templates_supported` to `false` on version change.
When the plugin's version changes and the options are empty (meaning no options were stored previously), then `set all_templates_supported` to `false`.

Why?

On version change and when:

1. no options were previously saved
2. switching from classic to either paired or native

then the settings page was blasted with erroneous admin notices.  See issue #1302.

This commit fixes that edge case.

Why not use `upgrader_process_complete` hook?  This hook works when updating from the WordPress.org plugin directory. However, when working with bleeding edge, we need to validate during the options manager init cycle.
@hellofromtonya

This comment has been minimized.

Copy link
Contributor

hellofromtonya commented Aug 14, 2018

Upon deeper investigation, the notice blasts occur when:

  1. There are no options saved in the database AND
  2. You switch from Classic to either Paired or Native.

It's not directly linked to upgrading, although it does occur when upgrading. At first I walked down that path to fix the upgrade process and setting all_templates_supported to false (i.e. see PR #1337). However, it occurs to me that if no options were saved in a previous version of the plugin, then the plugin is in a default state.

Now, I believe the root cause is in how or when we are validating the supported post types against the theme/plugin and what has been selected. There is a linkage between all_templates_supported and post type/template selections.

hellofromtonya added a commit that referenced this issue Aug 14, 2018

Use all eligible post types when all_templates_supported is selected.
Fixes #1302.

In the `check_supported_post_type_update_errors()` method, when `all_templates_supported` option is selected, we want to use all of the eligible post types and not just the ones that are individually selected.  In doing so, this commit fixes #1302 where it blasts erroneous notices about unable to deactivate supported post types.

@hellofromtonya hellofromtonya moved this from In progress to Ready for review in v1.0 Aug 14, 2018

@hellofromtonya hellofromtonya moved this from Ready for review to Ready for QA in v1.0 Aug 14, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 18, 2018

Moving To 'Ready For Merging'

If it's alright, I'm moving this to 'Ready For Merging.' It looks like reproducing this on the staging site will be complex, though we could create a new site for this if you think it's important.

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Sep 18, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

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.