Skip to content

Remove legacy config pages & related code#10669

Merged
brbrr merged 13 commits intomasterfrom
fix/remove-legacy-config-pages
Apr 11, 2019
Merged

Remove legacy config pages & related code#10669
brbrr merged 13 commits intomasterfrom
fix/remove-legacy-config-pages

Conversation

@brbrr
Copy link
Copy Markdown
Contributor

@brbrr brbrr commented Nov 19, 2018

Follow-up PR for cleaning up unused code after #10668

Changes proposed in this Pull Request:

  • Remove Jetpack::module_configuration_load(), Jetpack::module_configuration_head(), Jetpack::module_configuration_screen(), Jetpack::module_configuration_activation_screen() and relevant code.
  • Remove jetpack_module_configuration_head_{module}, jetpack_module_configuration_load_{module}, display_activate_module_setting_{module}, jetpack_notices_update_settings, jetpack_module_configuration_screen_{module} actions.
  • Remove usage of $_GET['configure'] and all relevant code.

Testing instructions:

Make sure that every module configuration link redirects to the desired location: (react) Jetpack Settings or wp-admin settings / other pages when applicable (for example for widgets):

  • All Configure links in the legacy modules admin screen.
  • Stats -> Configure link in header

Proposed changelog entry for your changes:

  • Admin page: Remove legacy settings pages and related code

@brbrr brbrr added [Status] In Progress Admin Page React-powered dashboard under the Jetpack menu [Type] Janitorial labels Nov 19, 2018
@brbrr brbrr self-assigned this Nov 19, 2018
@brbrr brbrr requested a review from a team November 19, 2018 16:16
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Nov 19, 2018

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: April 29, 2019.
Scheduled code freeze: April 22, 2019

Generated by 🚫 dangerJS against 37f1a44

@stale
Copy link
Copy Markdown

stale bot commented Feb 17, 2019

This PR has been marked as stale. This happened because:

  • It has been inactive in the past 3 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@stale stale bot added the [Status] Stale label Feb 17, 2019
@brbrr brbrr force-pushed the fix/remove-legacy-config-pages branch from de8240b to 3f91d0d Compare April 10, 2019 10:08
@stale stale bot removed the [Status] Stale label Apr 10, 2019
@brbrr brbrr force-pushed the fix/remove-legacy-config-pages branch from cbb7b48 to a3819f4 Compare April 10, 2019 13:12
@brbrr brbrr added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Apr 10, 2019
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This tests well for me. 👍

I would only recommend adding all the hooks that are now deprecated to deprecated_hooks, but that can be done in an additional PR.

@jeherve jeherve added this to the 7.3 milestone Apr 10, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Apr 10, 2019
@mdawaffe
Copy link
Copy Markdown
Member

I've manually added the relevant changes to code-D26734.

@mdawaffe
Copy link
Copy Markdown
Member

I would only recommend adding all the hooks that are now deprecated to deprecated_hooks

I didn't know that was a thing :)

Most of the hooks are dynamic: they contain the module slug. Any advice?

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Apr 10, 2019

they contain the module slug. Any advice?

That's a good question. I am not sure how we should handle, or if it should in fact. Maybe not that many third-parties are relying on those hooks today?

@georgestephanis
Copy link
Copy Markdown
Contributor

Maybe not that many third-parties are relying on those hooks today?

Probably. We had a tendency to add theoretical hooks optimistic that someone would find them useful, but it was rare that anyone ever did.

@georgestephanis
Copy link
Copy Markdown
Contributor

My only concern with this is if it breaks any no-js fallbacks, but we're probably okay with that. If the user uses the legacy modules page, is it possible to do some of this stuff any more?

@brbrr
Copy link
Copy Markdown
Contributor Author

brbrr commented Apr 11, 2019

@georgestephanis legacy settings pages are not easily accessible anymore (except stats settings), so there quite a small chance that anyone is still using them. You still can access them by passing the correct url-params though.

@brbrr brbrr merged commit a55e264 into master Apr 11, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 11, 2019
@brbrr brbrr deleted the fix/remove-legacy-config-pages branch April 11, 2019 11:02
@mdawaffe
Copy link
Copy Markdown
Member

is it possible to do some of this stuff any more?

Do you mean "is it still possible to change the settings that you used to be able to change via the legacy pages", then yes: to the best of my knowledge all settings available on the legacy pages are available in the React UI as well.

kraftbj added a commit that referenced this pull request Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Admin Page React-powered dashboard under the Jetpack menu Touches WP.com Files [Type] Janitorial

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants