Skip to content

Minileven: add options back as they still exist on sites#14756

Merged
jeherve merged 4 commits intomasterfrom
update/synced-options-minileven
Feb 25, 2020
Merged

Minileven: add options back as they still exist on sites#14756
jeherve merged 4 commits intomasterfrom
update/synced-options-minileven

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Feb 21, 2020

Changes proposed in this Pull Request:

This is a follow-up to #14714.
After removing those options, we started seeing errors like this one on sites:
PHP Warning: Invalid Jetpack option name: wp_mobile_excerpt

Since these Jetpack options are still valid options even if the feature is gone, let's keep them around.

Testing instructions:

  • Start from a site running Jetpack stable.
  • with the Beta plugin, switch from stable to this branch.
  • You should see no notices.

Proposed changelog entry for your changes:

  • N/A

@jeherve jeherve added Bug When a feature is broken and / or not performing as intended [Feature] Mobile Theme aka minileven [Status] Needs Review This PR is ready for review. [Package] Sync [Pri] Normal labels Feb 21, 2020
@jeherve jeherve added this to the 8.3 milestone Feb 21, 2020
@jeherve jeherve requested a review from a team February 21, 2020 16:18
@jeherve jeherve self-assigned this Feb 21, 2020
@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Feb 21, 2020
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Feb 21, 2020 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against 0060167

@jeherve
Minileven: add options back as they still exist on sites
523f4ed 
This is a follow-up to #14714.
After removing those options, we started seeing errors like this one on sites:
PHP Warning: Invalid Jetpack option name: wp_mobile_excerpt

Since these Jetpack options are still valid options even if the feature is gone, let's keep them around.
@jeherve jeherve force-pushed the update/synced-options-minileven branch from 8021449 to 523f4ed Compare February 21, 2020 18:02
@roccotripaldi
Copy link
Copy Markdown
Contributor

roccotripaldi commented Feb 21, 2020

Following your testing instructions, I did get a bunch of notices:


Warning: Invalid Jetpack option name: wp_mobile_excerpt in /srv/users/user116b676c/apps/user116b676c/public/wp-content/plugins/jetpack-dev/vendor/automattic/jetpack-options/legacy/class-jetpack-options.php on line 189

Warning: Invalid Jetpack option name: wp_mobile_featured_images in /srv/users/user116b676c/apps/user116b676c/public/wp-content/plugins/jetpack-dev/vendor/automattic/jetpack-options/legacy/class-jetpack-options.php on line 189

Warning: Invalid Jetpack option name: wp_mobile_app_promos in /srv/users/user116b676c/apps/user116b676c/public/wp-content/plugins/jetpack-dev/vendor/automattic/jetpack-options/legacy/class-jetpack-options.php on line 189

@kbrown9
Copy link
Copy Markdown
Member

kbrown9 commented Feb 21, 2020

It looks like the problem is that the three wp_mobile_* options that we delete aren’t managed by Jetpack_Options. So, Jetpack_Options::get_option() doesn’t recognize them as valid option names.

These options were originally in the array returned by Jetpack_Options::get_all_wp_options() The options in that array aren’t managed by Jetpack_Options.

I think we need to call core’s get_option() and delete_option() functions to get and delete these options in Jetpack::plugin_upgrade().

@kbrown9 kbrown9 added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Feb 21, 2020
@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 24, 2020
@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Feb 24, 2020

Thank you. That makes sense now, I was confused. I updated the calls, this should be ready for another review now.

@kraftbj kraftbj self-requested a review February 24, 2020 17:39
kraftbj
kraftbj previously requested changes Feb 24, 2020
View reviewed changes
@kraftbj kraftbj added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Feb 24, 2020
@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Feb 24, 2020

As discussed in Slack, we need to take into account the spun-out plugin so need to gate this to only delete options if the plugin is not active.

@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Feb 24, 2020

we need to take into account the spun-out plugin so need to gate this to only delete options if the plugin is not active.

I gave this a try in 0060167. Let me know if this is what you had in mind!

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 24, 2020
kbrown9
kbrown9 approved these changes Feb 25, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@kbrown9 kbrown9 left a comment

Choose a reason for hiding this comment

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

This looks good to me! The PHP warnings are no longer generated.

@jeherve jeherve dismissed kraftbj’s stale review February 25, 2020 06:25

Changes were addressed

@jeherve jeherve merged commit 23cbea4 into master Feb 25, 2020
@jeherve jeherve deleted the update/synced-options-minileven branch February 25, 2020 06:25
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review This PR is ready for review. labels Feb 25, 2020
@jeherve jeherve removed [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. labels Feb 25, 2020
jeherve added a commit to Automattic/jetpack-sync that referenced this pull request Feb 25, 2020
@jeherve
Minileven: add options back as they still exist on sites (#14756)
c9b7fa9 
* Minileven: add options back  as they still exist on sites

This is a follow-up to #14714.
After removing those options, we started seeing errors like this one on sites:
PHP Warning: Invalid Jetpack option name: wp_mobile_excerpt

Since these Jetpack options are still valid options even if the feature is gone, let's keep them around.

* Those options are not stored as Jetpack options.

See Automattic/jetpack#14756 (comment)

* Remove unneeded options

See https://github.com/Automattic/jetpack/pull/14756/files#r383413182

* Only delete options if the standalone plugin is not in use
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kbrown9 kbrown9 kbrown9 approved these changes

@kraftbj kraftbj kraftbj left review comments

Assignees

@jeherve jeherve

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Mobile Theme aka minileven [Package] Sync [Pri] Normal

Projects

None yet

Milestone

8.3

Development

Successfully merging this pull request may close these issues.

6 participants

@jeherve @jetpackbot @roccotripaldi @kbrown9 @kraftbj @matticbot