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

Composer 2 + AMP Plugin 2.1 / AMP PHP Toolbox #5344

Merged
merged 9 commits into from
Feb 18, 2021
Merged

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Nov 18, 2020

Summary

  • Moves the project to use Composer v2.
  • ...which is only supported by AMP plugin v2.1 (currently in development)
  • ...which in turn requires using AMP PHP toolbox from Packagist

Relevant Technical Choices

Note: This supersedes #5284

Since AMP plugin 2.1 hasn't been released yet, this loads the plugin from dev-develop instead.

To-do

  • Testing
  • Reduce dependency on AMP plugin itself so we can rely solely on AMP PHP Toolbox

User-facing changes

N/A

Testing Instructions

N/A


Fixes #5083
Fixes #4897

@google-cla google-cla bot added the cla: yes label Nov 18, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 18, 2020

Size Change: 0 B

Total Size: 1.44 MB

ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 877 B 0 B
assets/css/stories-dashboard.css 288 B 0 B
assets/css/web-stories-embed-block.css 615 B 0 B
assets/js/chunk-fonts-********************.js 44.8 kB 0 B
assets/js/chunk-web-stories-template-0-********************.js 11 kB 0 B
assets/js/chunk-web-stories-template-1-********************.js 11.3 kB 0 B
assets/js/chunk-web-stories-template-2-********************.js 10.9 kB 0 B
assets/js/chunk-web-stories-template-3-********************.js 10.6 kB 0 B
assets/js/chunk-web-stories-template-4-********************.js 12.7 kB 0 B
assets/js/chunk-web-stories-template-5-********************.js 7.14 kB 0 B
assets/js/chunk-web-stories-template-6-********************.js 10.2 kB 0 B
assets/js/chunk-web-stories-template-7-********************.js 10.1 kB 0 B
assets/js/chunk-web-stories-textset-0-********************.js 5.23 kB 0 B
assets/js/chunk-web-stories-textset-1-********************.js 6.79 kB 0 B
assets/js/chunk-web-stories-textset-2-********************.js 7.81 kB 0 B
assets/js/chunk-web-stories-textset-3-********************.js 15.3 kB 0 B
assets/js/chunk-web-stories-textset-4-********************.js 4.38 kB 0 B
assets/js/chunk-web-stories-textset-5-********************.js 5.64 kB 0 B
assets/js/chunk-web-stories-textset-6-********************.js 5.43 kB 0 B
assets/js/chunk-web-stories-textset-7-********************.js 10.3 kB 0 B
assets/js/edit-story.js 657 kB 0 B
assets/js/stories-dashboard.js 497 kB 0 B
assets/js/vendors~chunk-ffmpeg-********************.js 5.61 kB 0 B
assets/js/web-stories-activation-notice.js 68 kB 0 B
assets/js/web-stories-embed-block.js 19.7 kB 0 B

compressed-size-action

@swissspidy swissspidy marked this pull request as ready for review November 18, 2020 21:53
@swissspidy swissspidy added Pod: WP & Infra Type: Infrastructure Changes impacting testing infrastructure or build tooling labels Nov 18, 2020
@spacedmonkey
Copy link
Contributor

This didnt update me to composer 2.

You are already using composer version 1.10.17 (1.x channel).
 done!

@spacedmonkey
Copy link
Contributor

I needed to run

 composer self-update --2

Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

I was running 1.10.17 and didnt upgrade.

@swissspidy
Copy link
Collaborator Author

This didnt update me to composer 2.

What do you mean with this?

The PR just updates the plugin to be Composer 2 compatible.

@pierlon
Copy link
Contributor

pierlon commented Nov 19, 2020

Update on this: the AMP plugin will be releasing v2.0.7 today, which is a hotfix for ampproject/amp-wp#5609. Due to an unnoticed incompatibility with Composer v2, we're downgrading back to v1 until we have a solution in place to fix the reported issue.

@swissspidy swissspidy marked this pull request as draft November 19, 2020 20:55
@spacedmonkey spacedmonkey mentioned this pull request Nov 27, 2020
5 tasks
@spacedmonkey
Copy link
Contributor

@pierlon What is the status of Composer 2 in 2.0.8?

@swissspidy
Copy link
Collaborator Author

@pierlon What is the status of Composer 2 in 2.0.8?

@spacedmonkey Still using Composer 1 on the 2.0 branch. Upgrading to Composer v2 will be revisited for 2.1+.

@swissspidy swissspidy added the Status: Blocked On hold for the time being label Dec 21, 2020
@swissspidy swissspidy changed the title Composer 2 Composer 2 + AMP Plugin 2.1 / AMP PHP Toolbox Feb 4, 2021
@swissspidy
Copy link
Collaborator Author

@westonruter @pierlon Is there any rough ETA on the AMP plugin v2.1 release?

We're considering just using dev-develop and AMP Toollbox dev-main for the time being (see changes in this PR) so that we're not really blocked on you folks on improving our code base.

@westonruter
Copy link
Collaborator

@swissspidy We are planning to release v2.1 to coincide with WordPress 5.7. So roughly a month from now. If the prerelease state works for you, then I don't see any problem with that. Note that AMP Toolbox PHP should have a new stable release prior to the AMP plugin's v2.1. So you may only need to use dev-develop for the AMP plugin.

@pierlon Is there any dependency on ampproject/amp-wp#5745?

@pierlon
Copy link
Contributor

pierlon commented Feb 10, 2021

@pierlon Is there any dependency on ampproject/amp-wp#5745?

Yes, it would be of high priority now. At the moment if our patched version of the sabberworm/php-css-parser is not autoloaded first, it will impact the how the AMP plugin works internally. This wouldn't impact the Web Stories plugin, however, as it currently bundles the patched version of the CSS parser library and is scoped accordingly so that it always uses the one its shipped with.

@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #5344 (86c2036) into main (e6b68a8) will not change coverage.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5344   +/-   ##
=======================================
  Coverage   83.12%   83.12%           
=======================================
  Files        1106     1106           
  Lines       19758    19758           
=======================================
  Hits        16424    16424           
  Misses       3334     3334           
Flag Coverage Δ
karmatests 72.49% <ø> (-0.01%) ⬇️
unittests 65.76% <ø> (ø)

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

Impacted Files Coverage Δ
includes/Story_Renderer/HTML.php 72.94% <ø> (ø)
includes/AMP/Sanitization.php 83.87% <50.00%> (ø)

@swissspidy swissspidy removed the Status: Blocked On hold for the time being label Feb 17, 2021
@swissspidy swissspidy marked this pull request as ready for review February 17, 2021 08:29
Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

Worked well in PHP 7.3 and PHP 7.4 I tested locally. However, got error when installing on PHP 8.0. This is an issue in main, so I am going to make another issue to fix this elsewhere.

@swissspidy
Copy link
Collaborator Author

Worked well in PHP 7.3 and PHP 7.4 I tested locally. However, got error when installing on PHP 8.0. This is an issue in main, so I am going to make another issue to fix this elsewhere.

I just updated PHP-Scoper here which should fix this. Could you try again?

Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

Works in PHP 8.0.

@swissspidy swissspidy merged commit 59800e8 into main Feb 18, 2021
@swissspidy swissspidy deleted the add/composer-2 branch February 18, 2021 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Composer 2 Support Update AMP PHP Library Integration
4 participants