Remove modules infrastructure and UI from the plugin #1060

mukeshpanchal27 · 2024-03-18T09:15:03Z

Summary

Things needs to updates:

Document: https://make.wordpress.org/performance/handbook/performance-lab/releasing-the-plugin/

Note: In .github/workflows/deploy-standalone-plugins.yml, I've included some 'TODO' items that need to be addressed and removed before the merge.

mukeshpanchal27 · 2024-03-18T09:48:00Z

For the CODEOWNERS files:

Should we remove Focus areas?
Do we need to add a new section called Site Health, and include WebP Support Health Check, Autoloaded Options Health Check, and Enqueued Assets Health Check with their respective owners? What do you think?

github-actions · 2024-03-18T10:36:33Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: thelovekesh <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: joemcgill <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

.github/workflows/deploy-standalone-plugins.yml

plugins.json

readme.txt

admin/load.php

felixarntz

@mukeshpanchal27 This looks great so far. I left a few comments.

admin/load.php

plugins.json

readme.txt

load.php

thelovekesh · 2024-03-19T16:13:55Z

@mukeshpanchal27 #1033 caused merge conflicts 😐

.github/workflows/deploy-standalone-plugins.yml

admin/load.php

readme.txt

mukeshpanchal27 · 2024-03-20T05:15:16Z

@felixarntz, should we update the setting page slug from the existing /wp-admin/options-general.php?page=perflab-modules to /wp-admin/options-general.php?page=perflab-features?

thelovekesh · 2024-03-20T16:28:12Z

Overall changes look good to me. I just want to verify whether an updater script is required to remove any options or configurations added to the database by the previous PL plugin that will not be utilized in the next version.

felixarntz · 2024-03-20T17:29:22Z

@mukeshpanchal27

@felixarntz, should we update the setting page slug from the existing /wp-admin/options-general.php?page=perflab-modules to /wp-admin/options-general.php?page=perflab-features?

Great catch! I think this is a good idea, though I would suggest we go with the more generally applicable /wp-admin/options-general.php?page=performance-lab. Let's also rename the constant, e.g. to PERFLAB_SCREEN or PERFLAB_FEATURES_SCREEN.

It would be great to add another hook though which catches requests to /wp-admin/options-general.php?page=perflab-modules and redirects those to the new location, e.g. on the admin_page_access_denied action hook. For reference, consider this example in the Google Site Kit plugin.

@thelovekesh

I just want to verify whether an updater script is required to remove any options or configurations added to the database by the previous PL plugin that will not be utilized in the next version.

Great point as well! It would be great to implement an update routine which deletes the 'perflab_modules_settings' option if it is present. Since the option would be autoloaded anyway, I think we could maybe do that simply on admin_init if the current user has manage_options capabilities. This check should be cheap enough to run in that action hook, and it has the benefit that it works reliably at scale, even on large multisites.

westonruter · 2024-03-20T19:54:48Z

.github/workflows/php-test-standalone-plugins.yml

-      - name: Building standalone plugins
-        run: npm run build-plugins


Why are the plugins no longer needing to be built? In bin/plugin/commands/test-plugins.js it is not accounting for assets which may have been generated by Webpack as it is just copying the source files from /plugins to /build. It seems rather that test-plugins.js should be updated to reuse the output from npm run build-plugins which is already outputting the plugins into /build. cc @thelovekesh

Thank you for pointing that out. It's on my list to address as a follow-up PR since it scratches the current one.

Based on what @thelovekesh wrote in #1060 (comment), do we even need this anymore? The purpose of the test-plugins script and this GitHub workflow was purely to test whether standalone plugins don't cause some weird fatal error when built from their modules.

Since we don't build modules into plugins anymore, I think this isn't useful any more. For that reason, I would suggest to remove the whole test-plugins script and everything around it. Since this is only removing more things, it would be great if you could do it as part of this PR.

I think some of the code changes in this PR are a bit confusing because they're changing those scripts when they don't really have a good purpose anymore as a whole.

bin/plugin/commands/test-plugins.js

uninstall.php

mukeshpanchal27 · 2024-03-21T04:58:52Z

@westonruter, I'll update the test-plugins command in a follow-up PR.

@felixarntz @thelovekesh, the feedback has been addressed. The PR is now ready for another round of review.

thelovekesh

LGTM! Just a minor feedback ;)

load.php

thelovekesh · 2024-03-21T14:39:27Z

bin/plugin/commands/test-plugins.js

Do we still need this given we aren't converting modules to plugins anymore? I am not sure if we really need this.

That command simply checks if the plugin activates and doesn't return any errors, so I don't believe we need to remove it. I'd appreciate getting other opinions on this matter.

Just replied in #1060 (comment) :)

felixarntz

@mukeshpanchal27 Unless I'm missing something, I think we should remove the test-plugins script and everything using it, including the workflow, as this was intended to test plugins that were built from modules, which is no longer relevant. See https://github.com/WordPress/performance/pull/1060/files#r1534169130.

cc @westonruter @thelovekesh

westonruter · 2024-03-21T15:58:47Z

Unless I'm missing something, I think we should remove the test-plugins script and everything using it, including the workflow, as this was intended to test plugins that were built from modules, which is no longer relevant. See https://github.com/WordPress/performance/pull/1060/files#r1534169130.

I think there is a better way to check for fatals and that would be through E2E tests in #1051. I think it makes sense to remove the existing test-plugins script as it currently stands.

This can be done in a separate PR, AFAIK.

westonruter

Just a nit, but this looks like it can be merged and then further iterated on in subsequent PRs.

load.php

Co-authored-by: Weston Ruter <[email protected]>

felixarntz

I agree with @westonruter's assessment, let's merge this to unblock other PRs. We can remove the test-plugins command in a subsequent PR.

Initial changes

3ced295

mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release Creating standalone plugins labels Mar 18, 2024

mukeshpanchal27 self-assigned this Mar 18, 2024

Minor updates

00e660b

mukeshpanchal27 requested review from westonruter, joemcgill, swissspidy, felixarntz and thelovekesh March 18, 2024 09:36

mukeshpanchal27 marked this pull request as ready for review March 18, 2024 10:36

thelovekesh reviewed Mar 18, 2024

View reviewed changes

.github/workflows/deploy-standalone-plugins.yml Outdated Show resolved Hide resolved

plugins.json Show resolved Hide resolved

readme.txt Outdated Show resolved Hide resolved

admin/load.php Outdated Show resolved Hide resolved

felixarntz reviewed Mar 18, 2024

View reviewed changes

Address review feedback

66f0492

mukeshpanchal27 requested review from felixarntz and thelovekesh March 19, 2024 04:57

felixarntz reviewed Mar 19, 2024

View reviewed changes

.github/workflows/deploy-standalone-plugins.yml Outdated Show resolved Hide resolved

admin/load.php Outdated Show resolved Hide resolved

readme.txt Outdated Show resolved Hide resolved

felixarntz mentioned this pull request Mar 19, 2024

Merge trunk into feature/image-loading-optimization #1063

Merged

mukeshpanchal27 added 3 commits March 20, 2024 10:04

Resolve merge conflict

dde4df9

Minor updates

079088b

Remove PERFLAB_MODULES_SETTING

3d406e5

Add missing end line

988aabc

westonruter requested changes Mar 20, 2024

View reviewed changes

mukeshpanchal27 added 3 commits March 21, 2024 10:11

Update page slug and delete settings option

59c96e5

Resolve merge conflict

35384d8

Fix spell

cd9f564

mukeshpanchal27 requested review from westonruter and felixarntz March 21, 2024 04:59

thelovekesh approved these changes Mar 21, 2024

View reviewed changes

load.php Outdated Show resolved Hide resolved

thelovekesh reviewed Mar 21, 2024

View reviewed changes

Remove get_option call

959377c

felixarntz reviewed Mar 21, 2024

View reviewed changes

westonruter approved these changes Mar 21, 2024

View reviewed changes

load.php Outdated Show resolved Hide resolved

Enhance testability of redirect.

fe8f9f6

Co-authored-by: Weston Ruter <[email protected]>

felixarntz approved these changes Mar 21, 2024

View reviewed changes

felixarntz added 2 commits March 21, 2024 09:19

Merge branch 'trunk' into fix/1030-remove-modules-infrastructure

a91b3fb

Remove extra whitespace.

c40dde2

felixarntz changed the title ~~Remove modules infrastructure~~ Remove modules infrastructure and UI from the plugin Mar 21, 2024

felixarntz added this to the performance-lab 3.0.0 milestone Mar 21, 2024

felixarntz removed the no milestone PRs that do not have a defined milestone for release label Mar 21, 2024

felixarntz merged commit fb3553c into trunk Mar 21, 2024
43 checks passed

felixarntz deleted the fix/1030-remove-modules-infrastructure branch March 21, 2024 16:30

This was referenced Mar 21, 2024

Remove modules infrastructure #1030

Closed

Remove workflow to test standalone plugins built from modules as no longer relevant #1071

Merged

mukeshpanchal27 mentioned this pull request Apr 8, 2024

Delete option when uninstalling the Modern Image Formats plugin #1116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove modules infrastructure and UI from the plugin #1060

Remove modules infrastructure and UI from the plugin #1060

mukeshpanchal27 commented Mar 18, 2024 •

edited

Loading

mukeshpanchal27 commented Mar 18, 2024

github-actions bot commented Mar 18, 2024 •

edited

Loading

felixarntz left a comment

thelovekesh commented Mar 19, 2024

mukeshpanchal27 commented Mar 20, 2024

thelovekesh commented Mar 20, 2024

felixarntz commented Mar 20, 2024 •

edited

Loading

westonruter Mar 20, 2024

mukeshpanchal27 Mar 21, 2024

felixarntz Mar 21, 2024

mukeshpanchal27 commented Mar 21, 2024

thelovekesh left a comment

thelovekesh Mar 21, 2024

mukeshpanchal27 Mar 21, 2024

felixarntz Mar 21, 2024

felixarntz left a comment

westonruter commented Mar 21, 2024 •

edited

Loading

westonruter left a comment

felixarntz left a comment

		- name: Building standalone plugins
		run: npm run build-plugins

Remove modules infrastructure and UI from the plugin #1060

Remove modules infrastructure and UI from the plugin #1060

Conversation

mukeshpanchal27 commented Mar 18, 2024 • edited Loading

Summary

mukeshpanchal27 commented Mar 18, 2024

github-actions bot commented Mar 18, 2024 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

thelovekesh commented Mar 19, 2024

mukeshpanchal27 commented Mar 20, 2024

thelovekesh commented Mar 20, 2024

felixarntz commented Mar 20, 2024 • edited Loading

westonruter Mar 20, 2024

Choose a reason for hiding this comment

mukeshpanchal27 Mar 21, 2024

Choose a reason for hiding this comment

felixarntz Mar 21, 2024

Choose a reason for hiding this comment

mukeshpanchal27 commented Mar 21, 2024

thelovekesh left a comment

Choose a reason for hiding this comment

thelovekesh Mar 21, 2024

Choose a reason for hiding this comment

mukeshpanchal27 Mar 21, 2024

Choose a reason for hiding this comment

felixarntz Mar 21, 2024

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

westonruter commented Mar 21, 2024 • edited Loading

westonruter left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

mukeshpanchal27 commented Mar 18, 2024 •

edited

Loading

github-actions bot commented Mar 18, 2024 •

edited

Loading

felixarntz commented Mar 20, 2024 •

edited

Loading

westonruter commented Mar 21, 2024 •

edited

Loading