Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

Status: Active » Needs review
FileSize
6.64 KB

patch, lets see what testbot thinks.

phenaproxima’s picture

Status: Needs review » Needs work

Looks good to me, but can this also include updated dumps for D7?

The last submitted patch, 2: all-users-in-migrate-dump.patch, failed testing.

benjy’s picture

We did this on purpose because we didn't want uid 0 and 1 in the dumps since they're automatically created when you install your D8 site. ID's *should* be preserved because things like user ids are often important. Take for example a drupal.org upgrade, I don't think everyone would like getting a new user id.

If we had another way around ignore user 0/1 then we could do this but from what i'm aware, it only affects people altering the dumps.

phenaproxima’s picture

Yes, uid 0 and 1 are created when you install a D6/7 site, but that sidesteps the purpose of migrate-db.sh, which is to dump or restore a completely functional D6 or D7. Drupal never installed -- not by people who use migrate-db.sh to create fixtures, and not by the tests.

mikeryan’s picture

More context in the related issue...

benjy’s picture

Discussed on IRC, I think the ids were removed to get around an issue which i think the current patch shows and I think the issue Mike linked shows how to fix that.

neclimdul’s picture

The actually problem causing the failures with the current patch is mysql treating 0 as "use autoincrement". SET SQL_MODE="NO_AUTO_VALUE_ON_ZERO" is the fix but its database specific so this is hairy. mike linked #204411: Anonymous user id breaks on MySQL export/import (xID of Zero get's auto-incremented on external xSQL events) in IRC which discusses this. Since auto increment starts at 1, the first insert creates uid 1. then the insert with uid 1 fails.

As noted in the discussion on the other issue, not dumping uid 1 is actually just a weirdness. we skip it anyways in migrations so we're just making things weird for people manual testing and using the migrate script to build patches.

phenaproxima’s picture

Title: Migrate-db.sh skips uid 0 and 1 but shouldn't » migrate-db.sh skips uid 1 but shouldn't
Status: Needs work » Needs review
FileSize
3.3 KB

Try again...

Status: Needs review » Needs work

The last submitted patch, 10: 2562695-10.patch, failed testing.

phenaproxima’s picture

neclimdul’s picture

That explains why your patch sizes are odd.

mikeryan’s picture

+++ b/core/modules/user/src/Plugin/migrate/source/d6/User.php
@@ -25,7 +25,7 @@ class User extends DrupalSqlBase {
+      ->condition('uid', 0, '>');

Ooh, let's save that for #2560637: Improve handling of uid 1 during migration. If we commit this then D6 upgrades will unconditionally overwrite the admin account.

phenaproxima’s picture

The last submitted patch, 10: 2562695-10.patch, failed testing.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

lgtm

The last submitted patch, all-users-in-migrate-dump.patch, failed testing.

neclimdul’s picture

+1 RTBC

phenaproxima’s picture

Issue tags: +Migrate critical, +blocker

This is blocking a Migrate critical: #2560637: Improve handling of uid 1 during migration.

The last submitted patch, all-users-in-migrate-dump.patch, failed testing.

benjy’s picture

+1 from me.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

1000 yays for this patch, tho I'm confused about this hunk in core/modules/user/src/Tests/Migrate/d6/MigrateUserTest.php:

+      ->condition('uid', 1, '>')

Why do we deliberately add UID 1 info to the dump, but then deliberately skip testing it in the tests?

mikeryan’s picture

Why do we deliberately add UID 1 info to the dump, but then deliberately skip testing it in the tests?

The actual source plugin is skipping uid 1, so the test needs to skip it for now. As for why we have not changed the source plugin to now handle uid 1, there's more to that than just putting it through the pipeline as if it were a normal account, which we will deal with in #2560637: Improve handling of uid 1 during migration,

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

What he said! Restoring RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thanks for the response. See you over in #2560637: Improve handling of uid 1 during migration then!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed e882762 on 8.0.x
    Issue #2562695 by phenaproxima, neclimdul, mikeryan, benjy: migrate-db....

Status: Fixed » Closed (fixed)

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