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

Move published modules to standalone plugins #1011

Merged
merged 40 commits into from
Mar 5, 2024
Merged

Conversation

thelovekesh
Copy link
Member

Summary

  • Move published modules to the plugins directory. Moved modules include:
    • dominant-color-images
    • webp-uploads
  • Move modules test cases to tests/plugins directory.
  • Add testsuite entry in the PHPUnit config.
  • Update plugin files headers and doc blocks.
  • Update @since and @package tags.

Fixes #654

Relevant technical choices

  • Update @since tags based on the module changelog.
  • Update @package tags based on the module name.
  • Rename the main plugin file based on the plugin name.
  • Remove checks included to return if "module" was already activated.
  • Update plugin version as per plugin.json metadata.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@thelovekesh thelovekesh 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 labels Feb 23, 2024
@thelovekesh thelovekesh marked this pull request as ready for review February 23, 2024 18:40
Copy link

github-actions bot commented Feb 23, 2024

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: thelovekesh <thelovekesh@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: paaljoachim <paaljoachim@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>

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

.github/CODEOWNERS Show resolved Hide resolved
plugins.json Outdated
Comment on lines 13 to 7
"auto-sizes",
"dominant-color-images",
"webp-uploads"
Copy link
Member

Choose a reason for hiding this comment

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

Versions will need to be added once #1000 is merged.

plugins/dominant-color-images/.gitattributes Outdated Show resolved Hide resolved
plugins/webp-uploads/.gitattributes Outdated Show resolved Hide resolved
plugins/webp-uploads/helper.php Outdated Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
@thelovekesh
Copy link
Member Author

@mukeshpanchal27 JFYI recently added unit tests setup for standalone plugins doesn't seem to be running multisite test cases - https://github.com/WordPress/performance/actions/runs/8027561041/job/21931681466?pr=1011. I am aiming to amend this setup in #1013.

@mukeshpanchal27
Copy link
Member

@mukeshpanchal27 JFYI recently added unit tests setup for standalone plugins doesn't seem to be running multisite test cases - https://github.com/WordPress/performance/actions/runs/8027561041/job/21931681466?pr=1011. I am aiming to amend this setup in #1013.

Great catch. It was indeed missed. I've added the missing configuration in PR #1014.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @thelovekesh for the PR. This PR is blocked by #1000 and #1002. Left some initial feedbacks.

default-enabled-modules.php Outdated Show resolved Hide resolved
plugins/dominant-color-images/dominant-color-images.php Outdated Show resolved Hide resolved
plugins/webp-uploads/webp-uploads.php Outdated Show resolved Hide resolved
plugins/dominant-color-images/dominant-color-images.php Outdated Show resolved Hide resolved
plugins/webp-uploads/webp-uploads.php Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@thelovekesh This looks good so far.

I'm putting a "Request changes" for now, just so that we don't merge it yet until the foundation has been completed. I'm worried that if we merge this already while the foundation is still being worked on in other PRs (e.g. #1002 that you highlighted) we'll run into merge conflicts and things may become too confusing. Let's merge the removal/migration of the modules as the last step.

@thelovekesh
Copy link
Member Author

@felixarntz @mukeshpanchal27 Since #1000 and #1002 are now merged, we can pick it up next?

@mukeshpanchal27
Copy link
Member

@thelovekesh, could you please rebase the PR as it seems to have conflicts?

@thelovekesh
Copy link
Member Author

@mukeshpanchal27 Resolved the conflicts, Can you please take a look now?

plugins/dominant-color-images/load.php Outdated Show resolved Hide resolved
plugins/webp-uploads/load.php Outdated Show resolved Hide resolved
plugins/dominant-color-images/load.php Show resolved Hide resolved
plugins/webp-uploads/load.php Show resolved Hide resolved
plugins/webp-uploads/helper.php Outdated Show resolved Hide resolved
plugins.json Outdated Show resolved Hide resolved
@felixarntz
Copy link
Member

Please take a look at #656 (comment) where we can coordinate the next steps on how to proceed with this PR and #1019.

@thelovekesh
Copy link
Member Author

thelovekesh commented Mar 5, 2024

One overall concern: It looks like you removed all the individual .gitattributes files. Don't we still need them to ensure the extra files aren't part of the plugin ZIP builds? We will at least need to exclude .wordpress-org and phpcs.xml.dist for every plugin.

@felixarntz I removed it since it looks like it's not being used. Since .gitattributes is git specific, I can't find any instances in the build/deploy setup where this is being used for standalone plugins or modules. As a result, I can see ignored files/directories are there in the published plugin as well - https://plugins.svn.wordpress.org/dominant-color-images/trunk/.

Also, we might need to switch from .gitattributes to .distignore(see #893 (comment)) since some WIP plugins have assets that are not in the git tree but need to be included in the plugin release.

@felixarntz
Copy link
Member

@thelovekesh .gitattributes is supported by the 10up build action, no need to mention it in any of our build/deploy workflows. That's why it's used in the root of the project for the main PL plugin as well. At some point in the past we used .distignore, but it was less practical as it's not considered by GitHub itself, as far as I can remember.

Let me take a look at the dominant-color-images plugin, I believe the assets are in that one because of the same bug happening in the past, and they would need to be manually deleted to no longer be in there.

Let's use .gitattributes for now, as we have been, and we can rethink this in the future.

@thelovekesh
Copy link
Member Author

@felixarntz I think the 10up deploy action only takes .gitattributes into account if BUILD_DIR is false. For the main plugin we don't pass any BUILD_DIR but for the standalone plugins we do. Can you please validate it once?

@felixarntz
Copy link
Member

@thelovekesh Oh, that's a great catch! So it looks like there's no way to use either of the two files for the 10up deploy workflow to automatically handle it?

Maybe we have to implement something in our own workflow that removes those files before triggering the 10up deploy workflow? 🤔

@thelovekesh
Copy link
Member Author

@felixarntz Yes, and I think instead of handling it with manual CLI script we should switch to a build tool to better handle bundling use cases.

As far as we are concerned about .distignore, it shouldn't hurt our workflow in any way. .gitattributes play nice when we only want to build the stuff that is in the git tree.


Coming to the release of standalone plugins, we need to move WPOrg assets to some place(maybe tmp?) and then pass it path in ASSETS_DIR. It should do the trick.

@felixarntz
Copy link
Member

@thelovekesh In the interest of unblocking other PRs, let's merge this PR now without the correct handling of the assets.

Similarly to what I outlined in #1008, it would be great if you could open another PR against trunk to fix specifically that bug, to ensure that development files and brand assets for the standalone plugins are not part of the builds and deployments.

@felixarntz felixarntz dismissed mukeshpanchal27’s stale review March 5, 2024 18:52

Feedback has been addressed.

@felixarntz felixarntz merged commit b5cc801 into trunk Mar 5, 2024
38 checks passed
@felixarntz felixarntz deleted the move/modules-to-plugins branch March 5, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the modules that were published as standalone plugins from the Performance Lab plugin
4 participants