Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
There's a 100% fail rate in Drupal\Tests\path\Kernel\Migrate\d6\MigrateUrlAliasTest for Postgres on 8.2.x and 8.3.x. See https://www.drupal.org/pift-ci-job/571613 for example.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#11 | 2843358-11.patch | 796 bytes | mallezie |
#8 | 2843358-8.patch | 790 bytes | alexpott |
#2 | 2843358-2.patch | 525 bytes | alexpott |
Comments
Comment #2
alexpottA simple postgres ordering issue. Whilst processing in the order of primary key does not matter it does make it consistent.
Comment #3
mikeryanWhile that's a quick fix, isn't the real problem that the test is asserting a specific pid for the result, when the pid matters not a bit? Looking at the test, I don't think it should be asserting the pid at all - if the source/alias/langcode combination is right, that's all we need to verify.
Comment #4
alexpottI'm not sure I agree. In D6 and D7 the table structure is:
This means there can be duplicates and I think we should at least seek to preserve the order of the original table.
Same with D8.
In fact in \Drupal\Core\Path\AliasStorage::lookupPathAlias() we do a
$select->orderBy('pid', 'DESC');
so in fact this is a significant fix and the test is valid.Comment #5
mradcliffeWe had to do similar for many of the other tests that assert based on an implied order (either in views or with a query).
I'm not familiar with this test or code, but I think that the patch in #2 is correct if the migration query depends on an implied order.
Comment #6
alexpott@mradcliffe it is not the migration query - it is the actual url alias storage and results from querying it that depend on the order.
Comment #7
dawehnerPossible other migrations sources without a sort criteria:
\Drupal\user\Plugin\migrate\source\d7\User::query
\Drupal\user\Plugin\migrate\source\d7\Role::query
\Drupal\user\Plugin\migrate\source\d6\UserPictureFile::query
\Drupal\user\Plugin\migrate\source\d6\User::query
\Drupal\user\Plugin\migrate\source\d6\ProfileFieldValues::query
\Drupal\user\Plugin\migrate\source\ProfileField::query
\Drupal\tracker\Plugin\migrate\source\d7\TrackerUser::query
Should we maybe look at more of them?
Comment #8
alexpottAdded a comment based on @dawehner's feedback in IRC. I think @dawehner is correct in #7 because whilst order by's are probably not necessary as they are here it eliminates a variance and makes it easier to reason about and compare source and target sites.
Comment #9
dawehnerGreat documentation. Thank you!
Comment #10
mallezieMinor nitpick: Missing 'order' here?
Comment #11
mallezieAnd fixed it.
Comment #12
dawehnerHa, nice catch!
Comment #15
catchBig +1 to #4, since we don't have a redirect system in core, it's that order by which ensures a replacement alias gets used when generating URLs consistently, rather than different each time (while all paths in the system will be accepted inbound).
Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!