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

Integrate PHP CSS Parser library into plugin and add method to build Composer compatible zip #5745

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Dec 28, 2020

Summary

See #5609
See #2649

This PR modifies the current build setup to produce a Composer compatible zip that can then be referred to in a Composer configuration file. To accomplish this, the following changes were made:

  • Integrate the PHP CSS Parser library into the plugin. To make this work I took inspiration from the Web Stories plugin and used a tool called PHP-Scoper to move the sabberworm/php-css-parser library to a new destination and prefix the namespaces, classes, functions and constants within it to prevent any naming collisions that may occur if another version of the library were to be loaded in the same PHP runtime. The scoped library is also accompanied with a custom Composer autoloader which is then called in amp.php to load it. All references to the PHP CSS Parser library within the plugin have been updated accordingly to use the scoped version.

  • Add a new npm script called build:composer to build a Composer compatible version of the plugin. This is quite trivial to accomplish now that we don't have to worry about the PHP CSS Parser library getting in the way. In essence, the build process has been modified to keep the composer.json file and also update it to include the version of the plugin being built. The version of the plugin has to be set in the file so that Composer knows what version to get when it is being installed as a package.

  • Upload the Composer compatible zip to GCS and add a link to the comment that has the list of links to download each build type.

At the moment, it does not seems possible to publish the plugin on Packagist unless the built assets are going to be committed to a release branch and tagged accordingly (there may be some drawbacks with that approach). Alternatively, we could perhaps take advantage of GCS and setup our own private Composer repository. Upon each release we could upload the Composer compatible zip to a directory dedicated for that use and one could then add a custom repository to their Composer config to reference that URL path. Here's an example of such a config:

{
    "require": {
        "ampproject/amp-wp": "^2.0"
    },
    "repositories": [
        {
            "type": "artifact",
            "url": "https://storage.googleapis.com/ampwp_github_artifacts/composer"
        }
    ]
}

Props to @swissspidy for implementing a similar setup in the Web Stories plugin which served as inspiration for what was needed to be done in this plugin.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@pierlon pierlon added Infrastructure Changes impacting testing infrastructure or build tooling WS:Core Work stream for Plugin core labels Dec 28, 2020
@pierlon pierlon added this to the v2.1 milestone Dec 28, 2020
@pierlon pierlon self-assigned this Dec 28, 2020
@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #5745 (23b4da1) into develop (5e65134) will increase coverage by 0.13%.
The diff coverage is 81.81%.

❗ Current head 23b4da1 differs from pull request most recent head 2786ae9. Consider uploading reports for the commit 2786ae9 to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #5745      +/-   ##
=============================================
+ Coverage      75.09%   75.22%   +0.13%     
+ Complexity      5847     5650     -197     
=============================================
  Files            234      210      -24     
  Lines          17667    16977     -690     
=============================================
- Hits           13267    12771     -496     
+ Misses          4400     4206     -194     
Flag Coverage Δ Complexity Δ
javascript 75.93% <ø> (-3.91%) 0.00 <ø> (ø)
php 75.20% <81.81%> (+0.31%) 5650.00 <0.00> (-197.00) ⬆️
unit ? ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
amp.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
includes/options/class-amp-options-manager.php 99.44% <ø> (+6.62%) 73.00 <0.00> (-2.00) ⬆️
...s/validation/class-amp-validated-url-post-type.php 66.18% <ø> (+0.90%) 446.00 <0.00> (+5.00)
includes/sanitizers/class-amp-style-sanitizer.php 87.46% <100.00%> (+0.90%) 449.00 <0.00> (-4.00) ⬆️
...ncludes/sanitizers/trait-amp-noscript-fallback.php 0.00% <0.00%> (-100.00%) 10.00% <0.00%> (ø%)
includes/embeds/class-amp-tumblr-embed-handler.php 11.76% <0.00%> (-82.53%) 5.00% <0.00%> (-8.00%)
includes/embeds/class-amp-base-embed-handler.php 24.13% <0.00%> (-24.80%) 11.00% <0.00%> (-10.00%)
assets/src/block-editor/helpers/index.js 29.10% <0.00%> (-10.19%) 0.00% <0.00%> (ø%)
includes/embeds/class-amp-core-block-handler.php 87.93% <0.00%> (-5.35%) 46.00% <0.00%> (-2.00%)
src/Infrastructure/ServiceBasedPlugin.php 83.57% <0.00%> (-2.64%) 50.00% <0.00%> (-6.00%)
... and 92 more

@pierlon pierlon changed the title Introduce new build command to generate Composer compatible zips Integrate PHP CSS Parser library into plugin and add method to build Composer compatible zip Dec 29, 2020
@pierlon pierlon marked this pull request as ready for review December 29, 2020 03:17
@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2020

Plugin builds for 2786ae9 are ready 🛎️!

@swissspidy
Copy link
Collaborator

Integrate the PHP CSS Parser library into the plugin.

Should the AMP PHP Toolbox dependency also be scoped? There could be conflicts with other plugins using it as well.

The version of the plugin has to be set in the file so that Composer knows what version to get when it is being installed as a package.

According to the docs, this version field in composer.json is "optional if the package repository can infer the version from somewhere, such as the VCS tag name in the VCS repository. In that case it is also recommended to omit it."

I take it that this is the case here?

At the moment, it does not seems possible to publish the plugin on Packagist unless the built assets are going to be committed to a release branch and tagged accordingly

Curious to learn what's a good solution here...

Would it be possible to upload this final ZIP to the Github release, and Composer will get it from there (unless using --prefer-source)?

@pierlon
Copy link
Contributor Author

pierlon commented Dec 30, 2020

Should the AMP PHP Toolbox dependency also be scoped? There could be conflicts with other plugins using it as well.

That could be done, yes. I'd only apply that to the non-Composer builds though, since it would only be beneficial in that context.

According to the docs, this version field in composer.json is "optional if the package repository can infer the version from somewhere, such as the VCS tag name in the VCS repository. In that case it is also recommended to omit it."

I take it that this is the case here?

Yes, the version field is necessary in this case as the zip would be retrieved from a non-VCS environment.

Would it be possible to upload this final ZIP to the Github release, and Composer will get it from there (unless using --prefer-source)?

That could be possible, but you would need a way to tell Composer which of those ZIPs in each release is Composer compatible. I wonder if we could take advantage of Satis in this case to generate a list of package release definitions that would then refer to those releases on GitHub. Here's an example of a GitHub repository that's also being used to host a Composer repository: https://github.com/eventum/composer/tree/master.

@pierlon pierlon force-pushed the fix/2649-composer-compatible-build branch from 3a2323b to 4d4b329 Compare December 30, 2020 08:17
@swissspidy
Copy link
Collaborator

Should the AMP PHP Toolbox dependency also be scoped? There could be conflicts with other plugins using it as well.

That could be done, yes. I'd only apply that to the non-Composer builds though, since it would only be beneficial in that context.

Why only for those builds? What if another plugin on my non-composerized WordPress site also uses AMP PHP Toolbox? I'd want to avoid such conflicts.

@westonruter westonruter modified the milestones: v2.1, v2.2 Apr 5, 2021
@westonruter
Copy link
Member

Moving this to v2.2 milestone.

# Conflicts:
#	Gruntfile.js
#	codecov.yml
#	composer.json
#	composer.lock
@westonruter westonruter modified the milestones: v2.2, v2.3 Nov 30, 2021
@westonruter westonruter modified the milestones: v2.3, v2.4 Dec 23, 2021
@westonruter westonruter removed this from the v2.4 milestone Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Changes impacting testing infrastructure or build tooling WS:Core Work stream for Plugin core
Projects
None yet
3 participants