Avoid error from sort in ValidateMigrationStateTestTrait [#3204461]

Problem/Motivation

A test in Commerce Migrate that checks the migration states is failing with

1) Drupal\Tests\commerce_migrate_ubercart\Kernel\Migrate\uc7\ValidateMigrationStateTest::testMigrationState
sort() expects parameter 1 to be array, null given

It happens here where on <code>sort($declared[MigrationState::NOT_FINISHED])

because sort($declared[MigrationState::NOT_FINISHED]) is NULL.

I have a memory of intending to fix that when I wrote the test trait, ValidateMigrationStateTestTrait.php, and obviously forgot to.

Steps to reproduce

Proposed resolution

Set a default for the two arrays in $declared to [].

     $declared[MigrationState::FINISHED] = [];
    $declared[MigrationState::NOT_FINISHED] = [];

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comment	File	Size	Author
#14	3204461-14.patch	1.08 KB	larowlan
#14	9.3.x: PHP 8.0 & MySQL 5.7 28,545 pass
#14	3204461-interdiff-14.txt	1.17 KB	larowlan
#9	3204461-9.patch	797 bytes	quietone
#9	9.2.x: PHP 7.3 & MySQL 5.7 28,533 pass
#9	interdiff--2-9.txt	703 bytes	quietone
#2	3204461-2.patch	818 bytes	quietone
#2	9.2.x: PHP 7.3 & MySQL 5.7 28,224 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

19 March 2021 at 05:57

quietone created an issue. See original summary.

Comment #2

quietone CreditAttribution: quietone as a volunteer commented 19 March 2021 at 05:58

Status:

Active

» Needs review

File	Size
3204461-2.patch	818 bytes
9.2.x: PHP 7.3 & MySQL 5.7 28,224 pass

Comment #3

quietone CreditAttribution: quietone as a volunteer commented 25 March 2021 at 04:29

Title:

ValidateMigrationStateTestTrait

» Avoid error from sort in ValidateMigrationStateTestTrait

Comment #4

quietone CreditAttribution: quietone as a volunteer commented 25 March 2021 at 23:27

Issue tags:

+Bug Smash Initiative

Comment #5

quietone CreditAttribution: quietone as a volunteer commented 25 March 2021 at 23:30

Issue summary:

View changes

Comment #6

mparker17

he/him

English

UTC-4

CreditAttribution: mparker17 at Brady's Meat & Deli commented 24 April 2021 at 15:57

Status:

Needs review

» Reviewed & tested by the community

I've been adding D7 (and D6) migrations for a bunch of contrib modules on a site I need to migrate to D8, and I've been running into this issue - see https://www.drupal.org/pift-ci-job/1979943 for example.

I'd really like to see this get in, and backported to D9.1, D9.0, and D8.9 as well.

I can confirm this patch fixes my tests. Boldly marking as RTBC. Thanks @quietone!

Comment #7

quietone CreditAttribution: quietone as a volunteer commented 25 April 2021 at 00:35

@mparker17, thanks.

Comment #8

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 26 April 2021 at 09:34

Status:

Reviewed & tested by the community

» Needs review

+++ b/core/modules/migrate_drupal/tests/src/Traits/ValidateMigrationStateTestTrait.php
@@ -91,6 +91,9 @@ public function testMigrationState() {
     $declared = [];
+    $declared = [];

it looks like we declare declared twice?

Comment #9

quietone CreditAttribution: quietone as a volunteer commented 26 April 2021 at 09:50

File	Size
interdiff--2-9.txt	703 bytes
3204461-9.patch	797 bytes
9.2.x: PHP 7.3 & MySQL 5.7 28,533 pass

Oh fudge, silly me.

Comment #10

mparker17

he/him

English

UTC-4

CreditAttribution: mparker17 at Brady's Meat & Deli commented 26 April 2021 at 11:57

Status:

Needs review

» Reviewed & tested by the community

Re-RTBC! :D

Comment #11

26 April 2021 at 11:57

Version:

9.2.x-dev

» 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #12

3 May 2021 at 08:02

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 9: 3204461-9.patch, failed testing. View results

Comment #13

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 3 May 2021 at 08:22

Status:

Needs work

» Reviewed & tested by the community

Restoring status after HEAD was broken.

Comment #14

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 22 May 2021 at 00:40

Status:

Reviewed & tested by the community

» Needs review

File	Size
3204461-interdiff-14.txt	1.17 KB
3204461-14.patch	1.08 KB
9.3.x: PHP 8.0 & MySQL 5.7 28,545 pass

I went to commit this, but after looking at the patch applied locally (with more context than the patch here shows) I think we can optimise this even more.

Rather than send it back for those changes, I just made them locally.

Does this look good to you @quietone @mparker17?

I realise 14 comments and multiple patches is a lot of churn for such a small change, but as the saying goes, its Drupal, if it's worth doing, it's worth overdoing.

Comment #15

quietone CreditAttribution: quietone as a volunteer commented 22 May 2021 at 01:07

I applied the patch locally and immediately said to myself "Oh, that looks better!". This is a nice improvement and it fixes the problem stated in the IS. RTBC +1.

Now do I set it back to RTBC or not? Let's wait awhile and see if someone else chimes in.

Comment #16

22 May 2021 at 07:42

larowlan committed 43eafc2 on 9.2.x

Issue #3204461 by quietone, larowlan: Avoid error from sort in...

larowlan committed 40dabff on 9.3.x

Issue #3204461 by quietone, larowlan: Avoid error from sort in...

Comment #17

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 22 May 2021 at 07:42

Version:	9.3.x-dev	» 9.2.x-dev
Status:	Needs review	» Fixed

Because this is such a trivial patch, and my change was accepted by quietone, I'm going to go outside the normal process and commit this instead of holding it up longer. I've already pushed back twice on a one line patch.

I ummed and ahhed about just fixing it without a patch and saying 'fixed on commit' like we do for obvious coding-standards issues. But felt it was better to put up a patch and get a +1 from quietone.

Committed 40dabff and pushed to 9.3.x. Thanks!

Backported to 9.2.x