Problem/Motivation

Explore needed workarounds to get a build to pass on 8.7 and 8.8 without deprecation suppression and evaluate.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3089178-14.commerce.patch10.02 KBmikelutz
#12 interdiff.3089178.11-12.txt857 bytesmikelutz
#12 3089178-12.commerce.patch43.06 KBmikelutz
#11 interdiff.3089178.9-11.txt815 bytesmikelutz
#11 3089178-11.commerce.patch42.22 KBmikelutz
#9 interdiff.3089178.8-9.txt488 bytesmikelutz
#9 3089178-9.commerce.patch42.25 KBmikelutz
#8 interdiff.3089178.6-8.txt7.13 KBmikelutz
#8 3089178-8.commerce.patch42.19 KBmikelutz
#6 interdiff.3089178.5-6.txt317 bytesmikelutz
#6 3089178-6.commerce.patch38.01 KBmikelutz
#5 interdiff.3089178.4-5.txt2.09 KBmikelutz
#5 3089178-5.commerce.patch37.7 KBmikelutz
#4 Interdiff.3089178.3-4.txt3.26 KBmikelutz
#4 3089178-4.commerce.patch36.3 KBmikelutz
#3 interdiff.3089178.2-3.txt943 bytesmikelutz
#3 3089178-3.commerce.patch33.04 KBmikelutz
#2 3089178-2.commerce.patch32.12 KBmikelutz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikelutz created an issue. See original summary.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
FileSize
3089178-2.commerce.patch32.12 KB

Initial patch from #2952526-13: Make the 8.7 build pass without deprecation warnings for a baseline, and a commit to filter those changes out once that issue gets in.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
FileSize
3089178-3.commerce.patch33.04 KB
interdiff.3089178.2-3.txt943 bytes

My patch put the drupalci file in the wrong place, oops.

So here's the baseline.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
FileSize
3089178-4.commerce.patch36.3 KB
Interdiff.3089178.3-4.txt3.26 KB

And here's a batch for 8.8 that should fix a lot.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
FileSize
3089178-5.commerce.patch37.7 KB
interdiff.3089178.4-5.txt2.09 KB

Last deprecation is actually in the address module. I opened #3089266: AddressDefaultFormatter must implement \Drupal\Core\Security\TrustedCallbackInterface as of Drupal 8.8, but you can safely skip that deprecation for now, assuming address will be updated before 9.0.x is supported.

Here's a patch to suppress that deprecation. We use the custom error handler here rather than using @group legacy and @expectedDeprecation for a few reasons:

  1. The affected tests are not legacy tests, typically @group legacy is for testing a project's internal deprecations, and should be removed along with the internal deprecated code that they test.
  2. @group legacy skips all deprecations including new ones that we may want to be informed about. The error handler skips only a specific deprecation that we know we want to suppress right now.
  3. adding @expectedDeprecation only tests that a specific deprecation is actually thrown and errors if it isn't, it does not complain if additional deprecations are thrown. Therefore using it here would cause the test to fail if/when a new address release is cut to fix the problem.
  4. adding the error handler to the base test allows us to skip that deprecation globally in one spot, and provides a place to inject other deprecations that we may want to skip in the future (note: This one was only triggered by Functional tests, so CommerceBrowserTestBase was sufficient, additional deprecations may need to go in the CommerceKernelTestBase if this ever comes up again)

This should cover tests on drupal.org. Travis is still complaining, I think because Travis is installing drupal/mailsystem as a dev dependency and d.org isn't, and there are additional deprecations around that. Going to test that next.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
FileSize
3089178-6.commerce.patch38.01 KB
interdiff.3089178.5-6.txt317 bytes

Adding mailsystem as a dev requirement...

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Status: Active » Needs work
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Status: Needs work » Needs review
FileSize
3089178-8.commerce.patch42.19 KB
interdiff.3089178.6-8.txt7.13 KB

Hopefully this covers mailsystem and makes travis happy..

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
FileSize
3089178-9.commerce.patch42.25 KB
interdiff.3089178.8-9.txt488 bytes

Coding standard fix..

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Status: Needs review » Needs work

No, of course I wasn't thinking. Those mailsystem deprecations are happening outside of my error handler in the browser test.. :-( back to the drawing board.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Status: Needs work » Needs review
FileSize
3089178-11.commerce.patch42.22 KB
interdiff.3089178.9-11.txt815 bytes

Looks like we need to wrap the entire setup.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
FileSize
3089178-12.commerce.patch43.06 KB
interdiff.3089178.11-12.txt857 bytes

Okay, last test. As usual, I ended up in a rabbit hole, so let me know if you want this broken down somehow. If you commit #2952526: Make the 8.7 build pass without deprecation warnings I can pull those changes out and shrink this down quite a bit.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

Committed #2952526: Make the 8.7 build pass without deprecation warnings, let's proceed :)

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
FileSize
3089178-14.commerce.patch10.02 KB

Reroll. Note builds still fail on 7.3 and 8.9 due to #3089327: Make the testsuite pass on PhpUnit 7.

  • bojanz committed b65dcc0 on 8.x-2.x authored by mikelutz 
    Issue #3089178 by mikelutz: Make the build pass against 8.8 without...
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Fixed

Opened #3089616: Require Drupal 8.8 with a note about removing workarounds added here.
Tweaked the new composer.json constraint to match the format of the other constraints.
And committed.

Thank you!

Status: Fixed » Closed (fixed)

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

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Issue tags: +Drupal 9 compatibility

Per a Slack discussion with Gábor Hojtsy regarding usage of D9 tags (Drupal 9, Drupal 9 compatibility, Drupal 9 readiness, etc.), "Drupal 9 compatibility" should be used for contributed projects that need updating and "Drupal 9" was the old tag for D8 issues before the D9 branch was ready. Doing tag cleanup here based on that discussion.