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.

Sign up for GitHub

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

Jump to bottom

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

Closed
westonruter opened this issue Aug 1, 2018 · 5 comments · Fixed by #1338
Closed

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

westonruter opened this issue Aug 1, 2018 · 5 comments · Fixed by #1338
Assignees
@hellofromtonya
Labels
Bug Something isn't working Release
Milestone
v1.0

Comments

@westonruter
Copy link
Member

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 the Bug Something isn't working label Aug 1, 2018
@westonruter westonruter added this to the v1.0 milestone Aug 1, 2018
@westonruter
Copy link
Member Author

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.

@hellofromtonya hellofromtonya self-assigned this Aug 9, 2018
@hellofromtonya
Copy link
Contributor

hellofromtonya commented Aug 10, 2018 •
edited
Loading

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
Copy link
Member Author

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 pushed a commit that referenced this issue Aug 14, 2018
Initialize all_templates_supported to false on version change.
f5922ce 
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
Copy link
Contributor

hellofromtonya commented Aug 14, 2018 •
edited
Loading

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 hellofromtonya mentioned this issue Aug 14, 2018
Closed
hellofromtonya pushed a commit that referenced this issue Aug 14, 2018
Use all eligible post types when all_templates_supported is selected.
62f4f94 
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 mentioned this issue Aug 14, 2018
Merged
@westonruter westonruter mentioned this issue Aug 16, 2018
Merged
@kienstra
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hellofromtonya hellofromtonya

Labels
Bug Something isn't working Release
Projects
None yet
Milestone
v1.0
Development

Successfully merging a pull request may close this issue.

Use all eligible post types when all_templates_supported is selected. ampproject/amp-wp
3 participants
@westonruter @kienstra @hellofromtonya