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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Component: postgresql db driver » migration system
Status: Active » Needs review
Issue tags: -migrate +PostgreSQL
FileSize
525 bytes

A simple postgres ordering issue. Whilst processing in the order of primary key does not matter it does make it consistent.

mikeryan’s picture

While 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.

alexpott’s picture

I'm not sure I agree. In D6 and D7 the table structure is:

| url_alias | CREATE TABLE `url_alias` (
  `pid` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `src` varchar(128) NOT NULL DEFAULT '',
  `dst` varchar(128) NOT NULL DEFAULT '',
  `language` varchar(12) NOT NULL DEFAULT '',
  PRIMARY KEY (`pid`),
  UNIQUE KEY `dst_language_pid` (`dst`,`language`,`pid`),
  KEY `src_language_pid` (`src`,`language`,`pid`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 |
 url_alias | CREATE TABLE `url_alias` (
  `pid` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'A unique path alias identifier.',
  `source` varchar(255) NOT NULL DEFAULT '' COMMENT 'The Drupal path this alias is for; e.g. node/12.',
  `alias` varchar(255) NOT NULL DEFAULT '' COMMENT 'The alias for this path; e.g. title-of-the-story.',
  `language` varchar(12) NOT NULL DEFAULT '' COMMENT 'The language this alias is for; if ’und’, the alias will be used for unknown languages. Each Drupal path can have an alias for each supported language.',
  PRIMARY KEY (`pid`),
  KEY `alias_language_pid` (`alias`,`language`,`pid`),
  KEY `source_language_pid` (`source`,`language`,`pid`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT='A list of URL aliases for Drupal paths; a user may visit...'

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.

| url_alias | CREATE TABLE `url_alias` (
  `pid` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'A unique path alias identifier.',
  `source` varchar(255) NOT NULL DEFAULT '' COMMENT 'The Drupal path this alias is for. e.g. node/12.',
  `alias` varchar(255) NOT NULL DEFAULT '' COMMENT 'The alias for this path. e.g. title-of-the-story.',
  `langcode` varchar(12) CHARACTER SET ascii NOT NULL DEFAULT '' COMMENT 'The language code this alias is for. if ''und'', the alias will be used for unknown languages. Each Drupal path can have an alias for each supported language.',
  PRIMARY KEY (`pid`),
  KEY `alias_langcode_pid` (`alias`(191),`langcode`,`pid`),
  KEY `source_langcode_pid` (`source`(191),`langcode`,`pid`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COMMENT='A list of URL aliases for Drupal paths. a user may visit…'   |

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.

mradcliffe’s picture

We 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.

alexpott’s picture

@mradcliffe it is not the migration query - it is the actual url alias storage and results from querying it that depend on the order.

dawehner’s picture

Possible 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
  • ... many more

Should we maybe look at more of them?

alexpott’s picture

Added 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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great documentation. Thank you!

mallezie’s picture

Status: Reviewed & tested by the community » Needs work
+    // The of the migration is significant since

Minor nitpick: Missing 'order' here?

mallezie’s picture

Status: Needs work » Needs review
FileSize
796 bytes

And fixed it.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Ha, nice catch!

  • catch committed 05b64e0 on 8.3.x
    Issue #2843358 by alexpott, mallezie, dawehner: Postgres fail in Drupal\...

  • catch committed 66c3d8d on 8.2.x
    Issue #2843358 by alexpott, mallezie, dawehner: Postgres fail in Drupal\...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Big +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!

  • catch committed 05b64e0 on 8.4.x
    Issue #2843358 by alexpott, mallezie, dawehner: Postgres fail in Drupal\...

  • catch committed 05b64e0 on 8.4.x
    Issue #2843358 by alexpott, mallezie, dawehner: Postgres fail in Drupal\...

Status: Fixed » Closed (fixed)

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