FieldableEntity::getFieldValues() does not guarantee that the returned field values are sorted by their delta

Undefined sort orders from PostGreSQL are a class of bugs near and dear to me. :-)

Technically we don't promise that those deltas are in any particular order, so technically calling code should be sorting them if needed, but I'm sure it probably doesn't.
Fortunately, we technically don't promise that those deltas are in any particular order, so can certainly sort them without breaking backwards compatibility.

I would prefer that we go with the sort in the database call, that feels more performant. I would also like to see the comment for the return value of getFieldValues() to be edited to say The raw field values, keyed and sorted by delta.

Adding orderBy in the database query instead of using ksort(). Also updating the doc as per #5 and a minor CS issue

/var/www/html/core/modules/migrate_drupal/tests/src/Kernel/d7/MultiValueFieldMigrationsTest.php:211:47 - Unknown word (Opaka)
/var/www/html/core/modules/migrate_drupal/tests/src/Kernel/d7/MultiValueFieldMigrationsTest.php:275:47 - Unknown word (Opaka)

CSpell: failed

This is a nice addition, I like it. And mikelutz has already shown there are no BC concerns.

1. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MultiValueFieldMigrationsTest.php
   @@ -0,0 +1,285 @@
   +class MultiValueFieldMigrationsTest extends MigrateDrupal7TestBase {
   

   This test file is doing a lot of assertions that are already done elsewhere. This is just a node migration and then testing the results. So, we can just add only those assertions that are related to the changes in FieldableEntity to \Drupal\Tests\node\Kernel\Migrate\d7\MigrateNodeCompleteTest::testNodeCompleteMigration. We avoid making a new kernel test and this patch will be smaller as well.

I noticed the issue is stuck for several months, so wanted to push it a little bit 😝
@huzooka, can you review to make sure it's enough and I didn't remove too much?

The test passed locally, so I'm moving this to "Needs review".

I ran MigrateNodeCompleteTest locally without the fix and as I suspected, the tests pass. This needs a failing test. Setting to NW for that. Maybe in the setUp() field rows could be reordered?

It should fail with PostgreSQL, let's see.

This also affects migrations from Drupal 6. I don't know if it's OK to bring that into scope? The fix is almost identical; and I've added what I think is the equivalent test.

My apologies, I used a nid instead of a vid in the D6 test! These should work.

I reviewed that patch and it all looks good to me.

I looked at the d6 and d7 databases and can confirm that the order of the field values being tested in the D6 and D7 tests are correct.
Comments are updated in d6/Node and FieldableEntity. I can't think where else that needs to be documented.
There is a failing test, \d7\MigrateNodeCompleteTest

I have one question, why didn't d6\MigrateNodeCompleteTest fail?

Oh, and the IS should be updated to indicate which of the two proposed resolutions has been implemented.

Thx.

I have one question, why didn't d6\MigrateNodeCompleteTest fail?

The rows in the test fixtures are ordered the 'expected' way round (i.e. with deltas ascending), so the records can come back the right way around without the explicit ordering. I experienced this issue even with MySQL, because my source database (D6!) happened to have data that was ordered the other way around, so the same solution applies as for the unexpected order from PostgreSQL.

Do we need to set up a new test fixture database with rows the 'wrong' way around? Feels like unnecessary work to me.

@james.williams, thanks for that. I agree, there is no need to alter the test fixtures.

My question has been answered, the code is good and there are tests.

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!