The version of masterminds/html5 used by this code is out of sync with the version used by Drupal 8+. This makes it hard to get everything working together and creates testing problems.

This is a tracking issue because the fix actually needs to happen in the lullabot/amp library. There is a PR there with the beginnings of the fix needed. Once that is fixed and a new version of the library created, the composer.json in this module should be updated to require the new version of the library.

See https://github.com/Lullabot/amp-library/pull/249.

CommentFileSizeAuthor
#13 3154863-update-masterminds-html5.patch728 byteskarens
#5 3154863-update-masterminds-html5.patch720 byteskarens
#4 3154863-update-masterminds-html5.patch789 byteskarens

Comments

KarenS created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Category: Task » Bug report

It doesn't make it hard, it makes it impossible to use with Drupal 9 :)

https://www.drupal.org/pift-ci-job/1737740

That version isn't compatible with symfony 4.

karens’s picture

karens commented

Moved to this PR, https://github.com/Lullabot/amp-library/pull/280, looks like it's working with all versions of PHP, now to see if it works when I use it on Drupal.

karens’s picture

karens commented
Status: Active » Needs review
StatusFileSize
new 3154863-update-masterminds-html5.patch789 bytes

Fingers crossed!

karens’s picture

karens commented
StatusFileSize
new 3154863-update-masterminds-html5.patch720 bytes

Now I think we can get rid of the extra cruft in require-dev that I had to add for tests to pass.

karens’s picture

karens commented

My suspicion is that the require failure for D9 is due to the fact that the test starts with the dev version of the code, which is currently uninstallable, and then fails. So I think that failure is to be expected.

It's possible this upgrade will be tricky for end users because they have a current implementation that is locked on the wrong version of masterminds, so composer may refuse to change it.

I don't think there's any alternative, this patch is the way forward.

  • KarenS committed 902a0a1 on 8.x-3.x 
    Issue #3154863 by KarenS: Update masterminds/html5 version in AMP...
karens’s picture

karens commented

Now I need to figure out a release strategy. This will break D8 sites but work on D9. If the patch gets into 8.9, it this will work on 8.9.

  • KarenS committed 68307d8 on 8.x-3.x 
    Issue #3154863 by KarenS: Revert Update masterminds/html5 version in AMP...
karens’s picture

karens commented

Reverting because it screws up tests. I guess I need a new branch so I can run different tests on each branch. And to make it more clear which version is required.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I think that was progress, it actually did run the tests now. The test fails might have been the MySQL version, make sure you use MySQL 5.7+.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Ah, does lullabot/amp 2 require a version of of masterminds7html5 that requires symfony4?

Tricky. Maybe you can require ^1.0 || ^2.0, then it will install a working version on D8 and D9.

karens’s picture

karens commented
StatusFileSize
new 3154863-update-masterminds-html5.patch728 bytes

Will this work? I have no idea.

  • KarenS committed d9ca3aa on 8.x-3.x 
    Issue #3154863 by KarenS, Berdir: Update masterminds/html5 version in...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

https://www.drupal.org/pift-ci-job/1738983

Woot! And 8.9 passed too.

THis is fixed then, I think?

karens’s picture

karens commented
Status: Needs review » Fixed

I went ahead and merged this and both D8 and D9 tests pass. So hopefully this is working!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.