Problem/Motivation

Follow-up to #2683421: Remove incremental and rollback options from the UI (and add them back when they are more stable) which removes both the rollback and incremental options since they were untested and had several significant bugs. The incremental migration feature is an essential part of the migration process for many sites, because new content may be created on a live site during the initial migration and prototyping. We should definitely support this option through the UI for it to be considered complete.

Since this was created the help text for the module specifically states that increment migrations are not supported. That needs to change.

Proposed resolution

Add back incremental migration support, with test coverage. Consider refactoring the form and batch process so the logic for it is not entangled with the initial migration and the rollback option.

The test will have to change from the original patch because at that time aggregator was not install in the source test fixture. In the original test an upgrade was performed, then aggregator installed, then an upgrade was performed, then tests the results. It is no longer possible to test incremental upgrade that way. The new test will run the upgrade, add content, then run the upgrade, the test results. However, this is again modified because auditing is not part of the upgrade process. The test flow is add content, upgrade, add more content, upgrade, test results. See #96

Refactoring the form is still needed but since this issue was created a new issue was created to do just that #2918761: Break up MigrateUpgradeForm into smaller forms. That was done as a followup to #2876085: Before upgrading, audit for potential ID conflicts. Therefor, refactoring the form and batch process will be done there.

Add documentation to the handbook, somewhere in Upgrading from Drupal 6 or 7 to Drupal 8. This could be a new page.

Update the help text at migrate_drupal_ui_help().

Remaining tasks

Answer question in #143
Update help text at migrate_drupal_ui_help()
Add documentation - This will be done in #2938353: Document incremental migration.
Add change record draft
Review change record
Commit

User interface changes

In #2683421: Remove incremental and rollback options from the UI (and add them back when they are more stable), the UI disallows incremental migrations. For just incremental migrations (before rollback is supported), we should possibly make it part of the confirmation form.

Before

This is the text shown on /upgrade when an upgrade has previously been run:

An upgrade has already been performed on this site. To perform a new migration, create a clean and empty new install of Drupal 8. Rollbacks and incremental migrations are not yet supported through the user interface. For more information, see the upgrading handbook.

After

This is the new text shown on /upgrade when an upgrade has previously been run:

An upgrade has already been performed on this site. To perform a new migration, create a clean and empty new install of Drupal 8. Rollbacks are not yet supported through the user interface. For more information, see the upgrading handbook.
Last upgrade: Tue, 01/16/2018 - 16:52

And the screenshot of the next text.

The module help text:

Before

Incremental upgrades
Incremental upgrades are not yet supported through the user interface.

After

The text above is no longer displayed

API changes

None.

Data model changes

None

CommentFileSizeAuthor
#161 diff-154-161.txt1.81 KBquietone
#161 2687843-161.patch32.56 KBquietone
#159 diff.txt1.55 KBquietone
#159 2687843-159.patch32.57 KBquietone
#154 interdiff.txt1.21 KBquietone
#154 2687843-154.patch32.57 KBquietone
#149 Selection_001.png14.16 KBquietone
#136 interdiff-132.135.txt1.06 KBpiggito
#135 interdiff-132.135.patch1.06 KBpiggito
#135 2687843-135.patch31.29 KBpiggito
#132 interdiff-130-132.txt1.54 KBjofitz
#132 2687843-132.patch31.39 KBjofitz
#130 interdiff-127-130.txt7.62 KBjofitz
#130 2687843-130.patch31.42 KBjofitz
#127 interdiff_125-127.txt7.28 KBheddn
#127 2687843-127.patch29.13 KBheddn
#125 interdiff.txt6.84 KBquietone
#125 2684689-125.patch28.5 KBquietone
#123 interdiff.txt9.47 KBquietone
#123 2684689-123.patch24.38 KBquietone
#120 Selection_002.png48.23 KBquietone
#120 Selection_001.png20.95 KBquietone
#118 interdiff.txt1.05 KBquietone
#118 2684689-118.patch25.15 KBquietone
#116 2684689-116.patch24.24 KBquietone
#103 interdiff.txt1.89 KBquietone
#103 2684689-102.patch25.15 KBquietone
#98 interdiff-96-98.txt3.13 KBjofitz
#98 2687843-98.patch25.22 KBjofitz
#96 interdiff.txt8.25 KBquietone
#96 2687843-96.patch26.42 KBquietone
#94 interdiff_91-94.txt3.68 KBheddn
#94 2687843-94.patch28.29 KBheddn
#91 interdiff_88-91.txt5.62 KBheddn
#91 2687843-91.patch28.21 KBheddn
#88 interdiff.txt17.24 KBquietone
#88 2687843-88.patch24.88 KBquietone
#85 interdiff.txt893 bytesquietone
#85 2687843-85.patch6.93 KBquietone
#82 2687843-82.patch6.92 KBheddn
#36 interdiff.txt2.11 KBquietone
#36 add_back_incremental-2687843-8.2.x-36.patch12.01 KBquietone
#34 add_back_incremental-2687843-8.2.x-34.patch11.29 KBmikeryan
#33 interdiff-2687843-29-33.txt2.09 KBquietone
#33 2687843-33.patch11.29 KBquietone
#31 interdiff-2687843-29-31.txt1.5 KBquietone
#31 2687843-31.patch11.29 KBquietone
#29 interdiff-2687843-24-29.txt2.91 KBquietone
#29 2687843-29.patch11.21 KBquietone
#24 interdiff.txt898 bytesquietone
#24 2687843-24-8.1.patch10.11 KBquietone
#19 interdiff.txt877 bytesmallezie
#19 2687843-19-8.2.patch10.03 KBmallezie
#15 2687843-14-8.1.patch9.97 KBquietone
#14 2687843-14-8.2.patch9.97 KBquietone
#6 2687843-6.patch9.78 KBalexpott
#6 4-6-interdiff.txt668 bytesalexpott
#4 2687843-4.patch9.78 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
alexpott’s picture

Status: Postponed » Needs review
FileSize
9.78 KB

Here's a start - reintroduces incremental and extends the test coverage to do one.

Status: Needs review » Needs work

The last submitted patch, 4: 2687843-4.patch, failed testing.

alexpott’s picture

abhishek-anand’s picture

Status: Needs review » Reviewed & tested by the community

Tested patch #6 manually from Drupal 6.38 to 8.1

Incremental migration worked for devel generated content (50 users, 50 nodes and taxonomy), except for the fact that the association of taxonomy terms with nodes didn't work.

I guess that issue is not a part of UI.

Ready to be RTBC IMO.

catch’s picture

Status: Reviewed & tested by the community » Needs review

The new fields are NULL after running updates:

MariaDB [d8]> SELECT * FROM block_content_field_revision;
+----+-------------+----------+-------+------------+-------------------------------+------------------+------------------+---------------+
| id | revision_id | langcode | info  | changed    | revision_translation_affected | default_langcode | revision_created | revision_user |
+----+-------------+----------+-------+------------+-------------------------------+------------------+------------------+---------------+
|  1 |           1 | en       | Booom | 1463483483 |                             1 |                1 |             NULL |          NULL |
+----+-------------+----------+-------+------------+-------------------------------+------------------+------------------+---------------+
1 row in set (0.00 sec)

There's no author for blocks themselves, so that can't be copied over, but is it OK for it to be NULL?

Also is it worth using the 'changed' timestamp to populate the 'revision_created' column?

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

I've tested this with a D7 site with custom blocks

  1. Run initial migration - block_content_field_revision has NULL for revision_user but not revision_created for both existing blocks
  2. Add a third block to the D7 site
  3. Run the incremental migration - all three blocks continue to have revision_user NULL, but not revision_created (which is identical to changes for all three

Note I'm running 8.2.x, so maybe something changed to ensure revision_created gets populated. Regardless, the behavior at the database level (which is the same when using drush migrate-upgrade) is unrelated to the UI, so restoring to RTBC.

Since this is not a bug report, should we move this issue to 8.2.x?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2687843-6.patch, failed testing.

The last submitted patch, 6: 2687843-6.patch, failed testing.

alexpott’s picture

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -695,14 +695,20 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
         if ($date_performed = $this->state->get('migrate_drupal_ui.performed')) {
    

    Rather than an if in the form we should probably we using #access so the form array items are always present.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -745,6 +751,35 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
    +    $database_state_key = \Drupal::state()->get('migrate.fallback_state_key', '');
    +    $database = \Drupal::state()->get($database_state_key, [])['database'];
    

    This looks really fragile. If migrate.fallback_state_key is not defined this will break.

  3. I think given Migrate is experimental doing this in 8.1.x is okay.
mikeryan’s picture

Issue tags: -Migrate critical

On the weekly Migration hangout we decided this is not a critical - UI enhancements can go in at any time.

quietone’s picture

Status: Needs work » Needs review
FileSize
9.97 KB

Rerolled this patch and found that it no longer applies to 8.2. Here are patches for both 8.1 and 8.2. But the 8.2 one will fail. How can I confirm that the entity counted detected are correct?

quietone’s picture

Hmm, seem to have forgotten to upload the patch for 8.1.x

The last submitted patch, 14: 2687843-14-8.2.patch, failed testing.

mallezie’s picture

Assigned: Unassigned » mallezie
Issue tags: +DevDaysMilan

I'll look into the 8.2 test fails.

mallezie’s picture

If i'm looking at the miscount correctly. This is probably since in the 8.2 branch the forum module is enabled in the MigrateUpgradeTestBase.

The counts after installing that module are off, by the number of entities forum module creates. This could be the culprit, but looking into it a bit more.

mallezie’s picture

Assigned: mallezie » Unassigned
FileSize
10.03 KB
877 bytes

I think this should do the trick for the 8.2 patch.

quietone’s picture

@mallezie, thanks. I forgot forum was added.

quietone’s picture

Re the comments in #12
1) That if is determining if batch_set was run, how would #access be used for that.
2) Can we just go to the credential form when the fallback is not set?

xjm’s picture

Issue tags: +Migrate critical

I'm actually going to override removing this from the Migrate critical list because incremental migration through the UI is a must-have to consider Migrate stable, not merely a UI enhancement. Site owners need to be able to do this without Drush and without starting an entirely new migration every time. (What is "Migrate critical" should not only be about when it can go in, but also about what the minimum needed functionality is for a stable Migrate suite.) So this issue is not a blocker for a stable API, but it is a required core functionality.

Thanks @mikeryan -- let me know if you still disagree and we can talk about it. :)

benjy’s picture

I think UI enhancements are pretty low on everyones list of priorities, hence that decision on the weekly call.

I think we need to find a maintainer who is both capable and willing to maintain the UI as from what I can tell, none of the current Migrate maintainers want to and I think that's fair enough given there is plenty to work on with the core API and the contrib tools.

quietone’s picture

Added a check so that if the database_state_key is not set, the credentials form will reload.

Note that on the call today I said I'd work on the UI issues. While I like back end work, I also believe these tools should be available for folks at all skill levels.

chx’s picture

Historical note: when in Prague we agreed on replacing the update path with migrate, the agreement was the UI will be a replacement of the update UI and nothing else. However, as we have just seen much more important international treaties get rewritten these days.

quietone’s picture

Thanks chx. I see very little about that in the issue queue. The closest are these two two https://www.drupal.org/node/2281691#comment-10338195, https://www.drupal.org/node/2701541#comment-11058525. Anyway, maybe this should be brought up on the next migrate call.

chx’s picture

The oldest I can find is https://groups.drupal.org/node/356283 one month after Prague. There might be a Google doc somewhere even older but not sure.

mikeryan’s picture

Status: Needs review » Needs work

I've tested this with my D6 personal blog site. I ran the upgrade process with a stale dev copy of the site (dating back to late 2014), then refreshed the local DB from the live site and ran the incremental process. This pulled in, for example, 1544 new user accounts (all spam), 13 new blog nodes (all right, I wasn't particularly prolific in that period), 40 new comments, etc. Mostly it went all right, but we have one major problem - all new files failed. The problem here is that the source_base_path from the original migration is not persisted anywhere (only injected at runtime to the file migration), and we don't reprompt on the incremental migration, so source_base_path isn't set on the incremental migration and I get a bunch of

Source ID 131: File '/sites/default/files/batch_rollback.jpg' does not exist.

So, I think we need to save the original source_base_path in state somewhere, unless anyone has a better idea...

quietone’s picture

Status: Needs work » Needs review
FileSize
11.21 KB
2.91 KB

Implemented storing source_base_path in state. Works locally on a quick test with devel generated content.

mikeryan’s picture

Status: Needs review » Needs work

Very close now...

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -740,6 +746,46 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
    +    $source_base_path = \Drupal::state()->get('migrate.source_base_path_state_key', '');
    +    if (!$source_base_path) {
    +      $form_state->setValue('step', 'credentials');
    +      $form_state->setRebuild();
    +    }
    

    It's allowed to omit the source_base_path, but this forces the user to go back through the credentials step if they did omit it on the initial migration. Let's just assign the source_base_path as-is.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -883,11 +928,22 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +  /**
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   * @param array $database
    +   * @param string $source_base_path
    +   * @param string $form_error_name
    +   */
    

    Please flesh out the function docs.

Note that the current patch does not apply to 8.2.x.

Thanks!

quietone’s picture

Status: Needs work » Needs review
FileSize
11.29 KB
1.5 KB

1-2. Yes, of course. Both fixed.

Status: Needs review » Needs work

The last submitted patch, 31: 2687843-31.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
11.29 KB
2.09 KB

Well, thats what happens when you're not working from HEAD.

mikeryan’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs review » Reviewed & tested by the community
FileSize
11.29 KB

Tested with both 8.1.x and 8.2.x (note the attached patch for 8.2.x - a simple reroll, so I feel I can still give RTBC). Ready as far as I'm concerned! The only question is whether to bother committing to 8.1.x at this point, especially considering other UI changes like #2702531: Edit UI text for Drupal Upgrade UI module have been committed to 8.2.x only (hence the need for a reroll)...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: add_back_incremental-2687843-8.2.x-34.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
12.01 KB
2.11 KB

The forum module is installed after the first migration, so the counts need to be adjusted to include both the forum and the aggregator modules
.

Status: Needs review » Needs work

The last submitted patch, 36: add_back_incremental-2687843-8.2.x-36.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review

Tests are passing now, 8.1.x in #33 and 8.2.x in #35.

Not setting back to RTBC due to the changes needed for 8.2.x.

almaudoh’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC per #34 and #38.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Version: 8.3.x-dev » 8.2.x-dev

Interesting; #2702531: Edit UI text for Drupal Upgrade UI module could be backported to 8.1.x since Migrate UI is experimental. However, this definitely should go into 8.2.x at the least, so moving back to that branch.

Thanks @quietone and @mikeryan!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: add_back_incremental-2687843-8.2.x-36.patch, failed testing.

xjm’s picture

Status: Needs work » Reviewed & tested by the community

Those fails look suspicious. Re-RTBCing and retesting.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: add_back_incremental-2687843-8.2.x-36.patch, failed testing.

quietone’s picture

Status: Needs work » Reviewed & tested by the community

The tests are passing. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: add_back_incremental-2687843-8.2.x-36.patch, failed testing.

quietone’s picture

Status: Needs work » Reviewed & tested by the community

No, tests are passing. Back to RTBC.

alexpott’s picture

I have a concern about adding back incremental migrations without rollback. One problem that incremental migrations brings to the fore is ID clashes, for example, node ID. Core's default migrations map the incoming node ID to the same node ID on the new site. With incremental migrations we make it extremely likely that a clash will occur. In this instance the incoming node will not be migrated and without rollback the user has no way to address this. With rollback the way to address it is to delete the node on the new site, do the migration and then re-create the node. I'm not sure if this is actually the desired outcome. I think what we really want is the option to preserve IDs where possible and generate new IDs if required.

mikeryan’s picture

Alex and I discussed the ID preservation issue some in #drupal-migrate today, with heddn helpfully reminding me we had previously brought up the fragility of ID preservation at #2748609: [meta] Preserving auto-increment IDs on migration is fragile. Full transcript below... My feeling is that attempting to preserve IDs without guaranteeing we're preserving all of them would be a mess. And, as I pointed out below, there's more to it than just generating new IDs in the context of a specific migration, we'd have to review all the migrations in core and make sure they properly use the migration process plugin to map the IDs (we have a fair amount of places that are taking for granted the IDs are preserved).

In general, on migration projects I always discourage people from trying to preserve IDs unless they absolutely must have them (in particular for SEO when not using pathauto everywhere - like, say, drupal.org). I recognize that's not practical for the basic upgrade scenario though, where there's a strong expectation of preservation.

From the related issue, the ideas I had for dealing with this:

  1. Document that if you plan on doing incremental migrations, DO NOT create stuff in D8.
  2. Set ridiculously high auto-increment values on the D8 tables so preserved IDs won't collide with new ones.
  3. ?

#2 could be documented for ADVANCED ADMINISTRATORS ONLY;).

tl;dr - I'm fine with this functionality going in as-is, perhaps adding some emphasis of the danger of manually adding content between migration runs - it will still be useful in many scenarios.

[2:43pm] alexpott: Is there a way to tell migrate to use the same ID if free but if it's not just generate a new one?
[2:43pm] alexpott: I think this is going to be important for incremental migrations
[2:43pm] mikeryan1: alexpott: Well, I'd say go all-in - if you aren't going to preserve all the IDs, what's the point of preserving some?
[2:44pm] alexpott: mikeryan1: well preserving is the default core behaviour
[2:44pm] neclimdul: i don't know if I'd look at it that way but the first one that misses is likely to cascade into missing all future migrations so it seems limited in usefulness
[2:44pm] mikeryan1: alexpott: If you really wanted to do that, you could implement a process plugin to apply to the ID mappings, which would return NULL when the ID already exists
[2:45pm] mikeryan1: alexpott: But, you've got to be careful not to do that when updating a previously-imported item, of course
[2:45pm] alexpott: mikeryan1: and once we add incremental migrate to UI we're going to have people running into some nasties
[2:46pm] alexpott: Because when people start - they might have an empty site so preserving NIDs works... but then they create some new nodes on the old site and some new nodes on the new site and then run incremental
[2:49pm] mikeryan1: For that case, it would be exceedingly hard to save people from themselves
[2:49pm] mikeryan1: Besides managing the ids for the current migration, a lot of the current migrations in core take preservation for granted, they would all have to be updated to use the migration process plugin
[2:50pm] mikeryan1: Which I'm absolutely for, because that makes them more generally useful for custom migrations where you typically don't try to preserve IDs
[2:51pm] heddn: alexpott: mikeryan1 i think you are looking for https://www.drupal.org/node/2748609
[2:51pm] Druplicon: https://www.drupal.org/node/2748609 => Preserving auto-increment IDs on migration is fragile #2748609: [meta] Preserving auto-increment IDs on migration is fragile => 5 comments, 2 IRC mentions
[2:51pm] mikeryan1: I still feel that the core upgrade-all-the-things is only useful in practice for a limited number of sites, like personal blogs
[2:52pm] mikeryan1: Most large, and pretty much all complex, sites will be customizing their migration paths - at least, once we get the tools built
[2:52pm] mikeryan1: Which I'd love to be spending my time on once we stabliize core
[2:53pm] mikeryan1: heddn: Ah yes, we've talked about this before...
[2:54pm] mikeryan1: alexpott: #2: "Set ridiculously high auto-increment values on the D8 tables so preserved IDs won't collide with new ones."
[2:55pm] alexpott: mikeryan1: yeah that's a nice solution if you know about it before
[2:56pm] mikeryan1: What I really want to see (in D9) is dumping auto-increment fields and using guids to ID everything
[2:56pm] alexpott: mikeryan1: yes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: add_back_incremental-2687843-8.2.x-36.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review

Rerunning tests, not sure that failure was relevant.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Test passed, back to RTBC.

xjm’s picture

Assigned: Unassigned » alexpott

Assigning to @alexpott based on his previous feedback for the issue.

xjm’s picture

Issue tags: +rc eligible

As a feature addition for an alpha experimental module, this is rc eligible per: https://www.drupal.org/core/d8-allowed-changes#rc

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The problem we have here is that incremental migrations open up a world where the user will have created new content on the new Drupal 8 site and and the old Drupal 6 or 7 site. With the current configuration I think that incremental migrations might be destructive. Ie. if you have 7 nodes in your source source site and do the migration and then have 7 nodes in your Drupal 8 site. You then go and create a node on the source site and create a different node on the Drupal 8 site. Run the migration -> the new Drupal 8 node will be overwritten.

At the very minimum this scenario needs testing.

alexpott’s picture

Assigned: alexpott » Unassigned
benjy’s picture

For what it is worth, i'm -1 on adding this to core at all. There was no concept of incremental changes in the d6 to d7 upgrade path. If you need incremental content migrations, then you should use the tools available in contrib.

quietone’s picture

There was no concept of incremental changes in the d6 to d7 upgrade path.

For me, that sums up why this should not be done

Maybe once the challenges in #50 and #56 are solved this could be implemented. But not now.

mikeryan’s picture

Priority: Major » Normal
Status: Needs work » Postponed
Issue tags: -Migrate critical, -rc eligible
Related issues: +#2748609: [meta] Preserving auto-increment IDs on migration is fragile

Essentially, this is (at least) postponed on #2748609: [meta] Preserving auto-increment IDs on migration is fragile, if not "Closed (won't fix)".

Since there's a strong feeling it shouldn't even be done, removing "Migrate critical" and downgrading.

xjm’s picture

For me, descoping this means we totally lied to everyone about what the advantage of converting from update.php to Migrate would be. Maybe this is partly a sunk-cost fallacy on my part, but incremental migration was supposed to be the thing that differentiated Drupal 8 upgrades from every Drupal upgrade before it.

To me, wontfixing this issue is essentially saying you only deserve that benefit if you can figure out how to do a migration with an untested, minimally supported contrib module extending drush, that you have no way to know you need or even to find, which core does not require either. At least update.php was 100% supported in core.

xjm’s picture

We say over and over "With Drupal 8, your old site can remain up and running while you iterate on the new site, and then you just make the switch when you're done with the upgrade."

Removing incremental migrations makes that statement false. Worse than false, because not only can you not do that, you have to figure out on your own how to have both your latest data, and your site's views. To me, if we descope this from core, we need to put views migrations back in scope. I think this is a much easier problem than that.

alexpott’s picture

Priority: Normal » Major
Issue tags: +Migrate critical

I think @catch's solution on #2748609: [meta] Preserving auto-increment IDs on migration is fragile looks like it could be quite sensible and work for 99% use-case and the one where it does not will be using custom migrations. I've been discussing this with @xjm and she points out that if we only support full migrate and rollback we'll delete people's views on rollback. Because they will depend on the fields added by the migration. Given that rollback is not going to offer us a way out we need need to come up with a fix for #2748609: [meta] Preserving auto-increment IDs on migration is fragile and do this - or come up with a views migration :)

I'm re-tagging with migrate critical because the above reasoning from @xjm.

benjy’s picture

For me, descoping this means we totally lied to everyone about what the advantage of converting from update.php to Migrate would be

Was this documented somewhere as one of the initial goals of the initiative, I wasn't involved at the beginning but i'd be interested to read it.

One completely different tack on this would be to add the CLI runner to core first. I believe if we made a good job of the CLI runner we would solve issues like this and #2748609: [meta] Preserving auto-increment IDs on migration is fragile and hopefully the UI would be much simpler and reuse much of the same code.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Issue tags: +Migrate UI

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

webchick’s picture

Hm. Is it? My understanding was Drupal Migrate UI only had to offer functional/feature parity with update.php for major version upgrades. We didn't have rollback/incremental functionality back then; it was all-or-nothing.

webchick’s picture

Note that I might be arguing against my pre-2017 self with that statement. :)

But if so, it might be worth re-discussing, now that Drupal 8 is about to be 2 years old, and the lack of a stable migration path in core is one of the top adoption blockers.

heddn’s picture

I'm fine with re-discussing. However, adding back incremental is much more trivial after we fix #2876085: Before upgrading, audit for potential ID conflicts. But we could also add it as a new feature, post stable in a point release.

heddn’s picture

Issue tags: -Migrate critical

Based on what I see in #2905741: *DRAFT* Proposed product goals for Drupal 8.5/8.6(+) core, I'm removing the migrate critical tag. It seems like we are OK with postponing rollback in the UI as a post-launch task.

webchick’s picture

Well. Bear in mind that's a DRAFT. :) I think it's probably the release managers' call whether this is migrate critical or not.

heddn’s picture

Tagging

heddn’s picture

Gábor Hojtsy’s picture

Priority: Major » Critical
Issue tags: -Needs release manager review +Migrate critical

In #61 and #63 release and framework manager feedback was provided, and that did not change in the past year as per the migrate meeting we are just having.

Promoting to critical on the migrate meeting (as part of promoting all Migrate critical issues to critical).

webchick’s picture

Ok, adjusted the roadmap to bring the rollback functionality back under 8.5.0 targets: https://www.drupal.org/core/roadmap#migrate

quietone’s picture

Issue summary: View changes

Updated the IS to include adding documentation to the handbook. Aside from being a nice idea in general, this is a to do item from #2561249: [Meta] Reorganization of (some) d.o. migration documentation, see #11.

quietone’s picture

Issue summary: View changes

Added the issue this is postponed on to the IS.

heddn’s picture

Status: Postponed » Active
Issue tags: -String change in 8.2.0

I think this can be unblocked now. And I think it is the last blocker for 'beta' stability for the UI. I've based that assessment on the guidance provided in https://www.drupal.org/core/experimental#beta. I distinguish between features and bugs. There's still some bugs open for the UI, but those would block full stability. Not a beta release.

heddn’s picture

Can we get some input from a release manager? This seems to be the remaining beta blocker for the UI. Take a look at #2905491: Mark Migrate Drupal UI as stable and the requirements for beta. But this is the last feature/task. The remaining open issues are non API changes that fix bugs.

heddn’s picture

still needs tests, but this gets things moving.

Status: Needs review » Needs work

The last submitted patch, 82: 2687843-82.patch, failed testing. View results

heddn’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review

Let's make sure we use the correct branch, shall we?

quietone’s picture

Two minor fixes.

quietone’s picture

Assigned: Unassigned » quietone

Assigning to myself to add the tests.

quietone’s picture

While working on this I noticed that MigrateUpgradeTestBase installs forum and book, which means that the test that they can be installed post migration is no longer working. These tests were added in #2692247: Pre-existing 'forum' content type prevents forum module installation and #2714497: Pre-existing 'book' content type prevents book module installation and were made ineffective in #2422229: D6->D8 Core block migrations missing settings tests which added forum and book to $modules.

Can someone else confirm that is correct? If it is, we need a issue to address that.

quietone’s picture

Assigned: quietone » Unassigned
FileSize
24.88 KB
17.24 KB

Modified the tests to not install aggregator for the first upgrade, then install it for the second. Works for D7 but for D6 there are errors in the d6_block migration. The error involves garland, which surprises me, but I have not investigated (holidays and all).

Status: Needs review » Needs work

The last submitted patch, 88: 2687843-88.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

It seems like a legit error in the tests. I'm working to reproduce it. For now, here's some feedback.

  1. +++ b/core/modules/migrate_drupal/tests/src/Traits/CreateTestContentEntitiesTrait.php
    @@ -57,7 +57,7 @@ protected function installEntitySchemas() {
       protected function createContent() {
    
    @@ -74,7 +74,13 @@ protected function createContent() {
    +  protected function createContentUpgradeTest() {
    

    Can we split this into separate "create" steps so we can call them individually or as a group?

  2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -187,78 +180,48 @@ public function testMigrateUpgrade() {
    +    // Need to update available an dmissing path lists.
    

    Nit. and missing.

heddn’s picture

This is the issue with aggregator:

+++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
@@ -167,8 +167,10 @@ public function build() {
    */
   public function getCacheTags() {
     $cache_tags = parent::getCacheTags();
-    $feed = $this->feedStorage->load($this->configuration['feed']);
-    return Cache::mergeTags($cache_tags, $feed->getCacheTags());
+    if ($feed = $this->feedStorage->load($this->configuration['feed'])) {
+      $cache_tags = Cache::mergeTags($cache_tags, $feed->getCacheTags());
+    }
+    return $cache_tags;
   }
 
heddn’s picture

Or more specifically, the source data fixture from d6 has aggregator/feed data in it for a block. That block won't be functional unless the aggregator module is enabled on the destination. Once it is enabled, it loads the feed from the block (which was migrated earlier). But since that feed hasn't been migrated (yet), it just dies a very loud death and kills the entire site. This is made worse, because the feed block from the d6 source data was loaded on every single page.

Status: Needs review » Needs work

The last submitted patch, 91: 2687843-91.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

Status: Needs review » Needs work

The last submitted patch, 94: 2687843-94.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
26.42 KB
8.25 KB

Since the source d6 site is configured with aggregator and thus 'aggregator' related blocks it doesn't seem right to run the upgrade without aggregator installed on d6, and d7. For me, it makes better sense to start with the source fixtures as they are, run the upgrade, add some content and run the upgrade again. Of course, the test adds content before the upgrade for the audit testing. So we have, add content, upgrade, add more content, upgrade. That is what this patch does, and it worked locally for both d6 and d7.

Status: Needs review » Needs work

The last submitted patch, 96: 2687843-96.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jofitz’s picture

Status: Needs work » Needs review
FileSize
25.22 KB
3.13 KB

Fixed test failures: removed calls to (now non-existent) createFeedContent() because the code that was in there is now back in createContent(), which is called on the next line.

Fixed coding standards errors.

phenaproxima’s picture

Status: Needs review » Needs work

Fairly shallow review, but I really like the extensive test coverage. Looks pretty good to me, overall.

  1. +++ b/core/modules/migrate_drupal/tests/src/Traits/CreateTestContentEntitiesTrait.php
    @@ -128,4 +128,61 @@ protected function createContent() {
    +      'uid' => 42,
    +      'name' => 'universe',
    

    Hah! :)

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -236,6 +244,51 @@ public function submitOverviewForm(array &$form, FormStateInterface $form_state)
    +    $database_state_key = $this->state->get('migrate.fallback_state_key', '');
    +    if ($database_state_key) {
    

    Why would we use the fallback state key here? The user has entered explicit database credentials here, so I don't understand why we wouldn't just use those.

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -236,6 +244,51 @@ public function submitOverviewForm(array &$form, FormStateInterface $form_state)
    +      $form_state->setValue('step', 'credentials');
    +      $form_state->setRebuild();
    

    Nit: setValue() is chainable, so we could do ->setValue()->setRebuild().

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -848,6 +877,44 @@ protected function getDatabaseTypes() {
    +   * Setup migrations.
    +   *
    +   * @param array $database
    +   *   Database array representing the source Drupal database.
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   *   The current state of the form.
    +   */
    +  protected function setupMigrations(array $database, FormStateInterface $form_state) {
    

    Can this doc comment be expanded a bit? "Setup migrations" tells me nothing more than the method name already does.

  5. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -848,6 +877,44 @@ protected function getDatabaseTypes() {
    +    $form_state->set('version', $version);
    +    $form_state->set('migrations', $migration_array);
    

    Nit: ->set() is chainable.

  6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -118,6 +118,12 @@ protected function tearDown() {
    +   * The upgrade is started three times. The first time is to test that
    +   * providing incorrect database credentials fails as expected. The second
    +   * time is to run the migration and assert the results. The third time is
    +   * to test an incremental migration, by installing the aggregator module,
    +   * and assert the results.
    

    Thanks for this!

  7. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -187,78 +181,50 @@ public function testMigrateUpgrade() {
    +    // Install aggregator module.
    +    \Drupal::service('module_installer')->install(['aggregator']);
    

    We should explain why we're doing this.

  8. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -187,78 +181,50 @@ public function testMigrateUpgrade() {
    +    $session->responseContains('An upgrade has already been performed on this site. To perform a new migration, create a clean and empty new install of Drupal 8. Rollbacks are not yet supported through the user interface.');
    

    This should probably be pageTextContains().

  9. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -315,4 +281,110 @@ protected function translatePostValues(array $values) {
    +   * Gets the expected number of entities per entity type after incremental.
    

    Should end with "...after incremental migration".

  10. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -315,4 +281,110 @@ protected function translatePostValues(array $values) {
    +   * @param $session
    +   *   The currenct session.
    

    $session needs a type hint (also in the method signature), and there's a typo in 'current'.

  11. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -315,4 +281,110 @@ protected function translatePostValues(array $values) {
    +   * @param $session
    +   *   The currenct session.
    +   */
    +  protected function assertIdConflict($session) {
    

    $session needs to be type hinted, and 'current' is misspelled. :)

  12. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -315,4 +281,110 @@ protected function translatePostValues(array $values) {
    +    // Have to reset all the statics after migration to ensure entities are
    

    'statics' should be 'static caches'.

quietone’s picture

A question, points 3, 4, 5, 10, 11, 12 are about existing code that is not changed in this patch. Should those changes be made here?

heddn’s picture

Re #100, Discussed in IRC with @phenaproxima and we're OK with punting those points of feedback until a follow-up.

heddn’s picture

Issue tags: +Migrate January 2017 Sprint
quietone’s picture

Status: Needs work » Needs review
FileSize
25.15 KB
1.89 KB

1. Glad you liked it
2. I an not totally sure, it has 'always' been that way. I can move that to the follow up as well. Chime in anyone who know why.
6. Your welcome
7. Removed. Aggregator, it is already installed via $modules in setup();
8. Done
9. Done, a bit awkward to trim ti 80 characters

heddn’s picture

Issue tags: -Migrate January 2017 Sprint +Migrate January 2018 Sprint
phenaproxima’s picture

Status: Needs review » Needs work

Close to godliness!

  1. +++ b/core/modules/migrate_drupal/tests/src/Traits/CreateTestContentEntitiesTrait.php
    @@ -128,4 +128,61 @@ protected function createContent() {
    +    $block = BlockContent::create([
    +      'info' => 'Post upgrade block',
    +      'type' => 'block',
    +    ]);
    +    $block->save();
    +
    +    // Create a node.
    +    $node = Node::create([
    +      'type' => 'page',
    +      'title' => 'Post upgrade page',
    +    ]);
    +    $node->save();
    

    We should check if each of these entity types actually exists before creating entities of those types, i.e., \Drupal::entityTypeManager()->hasDefinition('entity_type_id') around each of these ::create() calls.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -236,6 +244,51 @@ public function submitOverviewForm(array &$form, FormStateInterface $form_state)
    +      $form_state->setValue('step', 'credentials');
    +      $form_state->setRebuild();
    

    Nit: These calls are chainable, so we can write them as $form_state->setValue()->setRebuild().

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -236,6 +244,51 @@ public function submitOverviewForm(array &$form, FormStateInterface $form_state)
    +    $form_state->set('step', 'confirm_id_conflicts');
    +    $form_state->setRebuild();
    

    Same here.

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -848,6 +877,44 @@ protected function getDatabaseTypes() {
    +   * Setup migrations.
    +   *
    +   * @param array $database
    +   *   Database array representing the source Drupal database.
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   *   The current state of the form.
    +   */
    +  protected function setupMigrations(array $database, FormStateInterface $form_state) {
    

    Can we expand the doc comment to explain what this actually does?

  5. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -298,4 +261,110 @@ protected function translatePostValues(array $values) {
    +   * @param $session
    +   *   The currenct session.
    +   * @param $all_available
    +   *   Array of modules that will be upgraded.
    +   * @param $all_missing
    +   *   Array of modules that will not be upgraded.
    +   */
    +  protected function assertReviewPage($session, $all_available, $all_missing) {
    

    All three parameters should be type hinted, in both the method signature and the doc comment.

  6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -298,4 +261,110 @@ protected function translatePostValues(array $values) {
    +   * @param $session
    +   *   The currenct session.
    +   */
    +  protected function assertIdConflict($session) {
    

    $session should be type hinted in both spots.

  7. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -298,4 +261,110 @@ protected function translatePostValues(array $values) {
    +   * @param string $version
    +   *   The drupal verison.
    

    'drupal' should be capitalized, and this should be an int, not a string, since it's a numeric value.

  8. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -298,4 +261,110 @@ protected function translatePostValues(array $values) {
    +      $this->assertSame($expected_count, $real_count, "Found $real_count $entity_type entities, expected $expected_count.");
    +    }
    +    $plugin_manager = \Drupal::service('plugin.manager.migration');
    

    Nit: Can there be a blank line before $plugin_manager, for readability?

  9. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -298,4 +261,110 @@ protected function translatePostValues(array $values) {
    +        $source_id_values = array_values(unserialize($source_id));
    +        $row = $id_map->getRowBySource($source_id_values);
    

    I'm surprised that $id_map->getRowBySource() doesn't do the array_values(unserialize($source_id)) stuff. Seems like it might be a good idea. Follow-up, perhaps?

  10. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -298,4 +261,110 @@ protected function translatePostValues(array $values) {
    +        if ($row['source_row_status'] == MigrateIdMapInterface::STATUS_FAILED || $row['source_row_status'] == MigrateIdMapInterface::STATUS_NEEDS_UPDATE) {
    

    Why don't we replace this if statement, and the pass() and fail() calls, with a pair of assertions instead? Something like:

    $this->assertNotSame(MigrateIdMapInterface::STATUS_FAILED, $row['source_row_status']);
    $this->assertNotSame(MigrateIdMapInterface::STATUS_NEEDS_UPDATE, $row['source_row_status']);
    
anpolimus’s picture

Assigned: Unassigned » anpolimus
quietone’s picture

@anpolimus, are you working on this? Its just that I have time now and can pick it up.

quietone’s picture

@anpolimus, sorry, I was being over zealous.

anpolimus’s picture

@quietone I just want to make your comments fixed and prepare last version of the patch

anpolimus’s picture

working on it now

RytoEX’s picture

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
@@ -298,4 +261,110 @@ protected function translatePostValues(array $values) {
+   *   The drupal verison.

Nit: "verison" should be "version".

This caught my eye while reading #105 and wanted to write it down before I forgot.

Gábor Hojtsy’s picture

Version: 8.5.x-dev » 8.6.x-dev
Issue tags: +Needs issue summary update

Is UI feedback needed? (I tried to launch a simplytest.me but that failed due to a problem on their end I think). An updated issue summary would be nice if the UI is settled. It still says TBD.

As per https://groups.drupal.org/node/518148 this would be moved to 8.6. It may be backported on committer discretion to 8.5 still.

heddn’s picture

Assigned: anpolimus » Unassigned

Un-assigning for now so this critical doesn't stay blocked. @anpolimus if you have a patch, even if it is incomplete, please post it. But if not, let's see if someone else is able to jump on this.

This remains as the last issue blocker for the UI, so I want to see us keep momentum.

benjifisher’s picture

First, the patch from #103 needs a re-roll in addition to addressing the comments from #105 and #111. I think the code was moved around as part of #2914974: Migrate UI - handle sources that do not need an upgrade and is now in MigrateUpgradeExecuteTestBase.php instead of MigrateUpgradeTestBase.php.

When this issue is ready to review, it will need screenshots and an issue summary update.

quietone’s picture

Assigned: Unassigned » quietone

Working on the reroll.

quietone’s picture

Status: Needs work » Needs review
FileSize
24.24 KB

Only the reroll. Interdiff failed too.

Status: Needs review » Needs work

The last submitted patch, 116: 2684689-116.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Assigned: quietone » Unassigned
Status: Needs work » Needs review
FileSize
25.15 KB
1.05 KB

Try again.

quietone’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Reroll now working. Next is to address everything from #105 onwards.

quietone’s picture

Issue summary: View changes
FileSize
20.95 KB
48.23 KB

Updates IS a bit, added before and after screenshots.

quietone’s picture

Issue summary: View changes

Edit a sentence in the IS so it makes sense.

quietone’s picture

Remove the postponed section from the IS. This was made active in #80.
Add update the tags.

quietone’s picture

Another step on the way. This fixes #105.2-10, and #111.

That just leaves #105.1

quietone’s picture

Status: Needs work » Needs review
quietone’s picture

This should fix #105.1

phenaproxima’s picture

Status: Needs review » Needs work

I'm not seeing much to complain about here!

  1. +++ b/core/modules/migrate_drupal/tests/src/Traits/CreateTestContentEntitiesTrait.php
    @@ -60,72 +60,164 @@ protected function installEntitySchemas() {
    +    $entityTypeManager = \Drupal::entityTypeManager();
    

    Nit: Variable names inside methods should be $snake_case, not $camelCase.

  2. +++ b/core/modules/migrate_drupal/tests/src/Traits/CreateTestContentEntitiesTrait.php
    @@ -60,72 +60,164 @@ protected function installEntitySchemas() {
    +      $feed = Feed::create([
    +        'title' => 'feed',
    +        'url' => 'http://www.example.com',
    +      ]);
    +      $feed->save();
    

    Nit: We could make further use of $entity_type_manager, and avoid importing these entity classes, if we changed the format of these calls to $entity_type_manager->getStorage('entity_type_id')->create($values)->save();

  3. +++ b/core/modules/migrate_drupal/tests/src/Traits/CreateTestContentEntitiesTrait.php
    @@ -60,72 +60,164 @@ protected function installEntitySchemas() {
    +      $user = User::create([
    +        'uid' => 2,
    +        'name' => 'user',
    +        'mail' => 'user@example.com',
    +      ]);
    

    What if user 2 already exists? I don't know if we should be setting the uid explicitly here.

  4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -178,4 +179,107 @@ protected function assertUpgradePaths(WebAssert $session, array $available_paths
    +    $session->pageTextContains('custom block entities');
    +    $session->pageTextContains('custom menu link entities');
    +    $session->pageTextContains('file entities');
    +    $session->pageTextContains('taxonomy term entities');
    +    $session->pageTextContains('user entities');
    +    $session->pageTextContains('comments');
    +    $session->pageTextContains('content item revisions');
    +    $session->pageTextContains('content items');
    

    I wonder if we should pass in an array of entity type IDs, then assert that their plural labels appear on the page. That would be a more dynamic way to do this assertion. However, I'm OK with punting that to a follow-up.

heddn’s picture

Let's see how changing 126.1-3 breaks the tests. For 126.4, I've opened up #2937841: Assert plural labels exist on migrate upgrade form. I still see release manager review tagged here. Is that still needed?

Status: Needs review » Needs work

The last submitted patch, 127: 2687843-127.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jofitz’s picture

Assigned: Unassigned » jofitz
jofitz’s picture

Assigned: jofitz » Unassigned
Status: Needs work » Needs review
FileSize
31.42 KB
7.62 KB
  1. Corrected calls to getStorage().
  2. Moved some code to ensure variables always exist.
  3. Include sequences schema (which is necessary if uid is not provided when creating a user).
  4. Removed unused use statements.

Status: Needs review » Needs work

The last submitted patch, 130: 2687843-130.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
31.39 KB
1.54 KB

Dunno how I missed those ones too!

benjifisher’s picture

The IS still needs some work:

API changes
Depends on the solution.

Is the "Remaining tasks" section up to date?

Please add the new message as text. That will be more accessible than the screenshot, and easier to read for everyone.

phenaproxima’s picture

Status: Needs review » Needs work

Only one thing, then I think I'm happy with it.

+++ b/core/modules/migrate_drupal/tests/src/Traits/CreateTestContentEntitiesTrait.php
@@ -60,72 +50,163 @@ protected function installEntitySchemas() {
+      // Create an aggregator feed item.
+      if ($feed->id() && $entity_type_manager->hasDefinition('aggregator_item')) {
+        $item = $entity_type_manager->getStorage('aggregator_item')->create([
+          'title' => 'feed item',
+          'fid' => $feed->id(),
+          'link' => 'http://www.example.com',
+        ]);
+        $item->save();
+      }

We don't need the $feed->id() check; $feed->save() will throw an exception if saving fails. We also don't need the hasDefinition() check, because if aggregator_feed entities exist, so do aggregator_items.

piggito’s picture

Fixing the observation on #134

piggito’s picture

FileSize
1.06 KB

Uploaded interdiff with wrong extension by mistake so reuploading it.

Status: Needs review » Needs work

The last submitted patch, 135: interdiff-132.135.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review

Ignore test failure - that was just the interdiff.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

And it's green. Therefore, let it be greener. Thanks all!

benjifisher’s picture

I still think we need an IS update. See #133.

Also, I do not like relying on exceptions instead of using tests. Where will that exception be caught?

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

Also, I do not like relying on exceptions instead of using tests. Where will that exception be caught?

Which part of the patch are you referring to, specifically?

Agreed about the IS update; kicking back to review for that.

phenaproxima’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Having read this patch a few times, I don't see anything that counts as an API change. I've updated the IS accordingly.

Back to RTBC!

benjifisher’s picture

@phenaproxima: I was thinking of your comment in #134:

We don't need the $feed->id() check; $feed->save() will throw an exception if saving fails.

Then the patch in #135 removed the check.

Or maybe the two parts of that comment are independent, in which case we should add a try/catch block.

quietone’s picture

Issue summary: View changes

Added the new text to the IS as requested in #133.

benjifisher’s picture

Issue summary: View changes

Is this still accurate?

Remaining tasks
Add back incremental migration support, with test coverage.
Add documentation.

quietone’s picture

Issue summary: View changes

@benjifisher, thanks

Updated the remaining tasks. Can the documentation be done in a follow up or does it need to happen here before commit?

benjifisher’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Under "Proposed resolution", the IS refers to documentation in the handbook. If you want to defer this, then I think it is OK to create an issue for that and add a link to the IS. In fact, it is a little awkward to add documentation now to the handbook if the current patch will not be part of an official release until 8.6.0.

Luckily, we have resources that are more definitive than my opinion. The Drupal core gates include standards for documentation. Since this patch changes the UI, it needs in-code documentation:

  1. Docblocks for new or changed functions
  2. *.api.php if the patch adds new hooks (does not apply here, does it?)
  3. Update to hook_help() implementation
  4. A change record

Has migrate_drupal_ui_help() been updated? I do not see any mention of a change record on this issue, so I will add a tag for that and move the issue back to NW.

quietone’s picture

Issue summary: View changes

The migrate_drupal_ui_help() has not been updated, that needs to be done.

This is the current text on the help page about incremental. The original issue that removed rollback and incremental didn't not have any help test. We get to start from scratch.

Incremental upgrades
    Incremental upgrades are not yet supported through the user interface.
quietone’s picture

Issue summary: View changes
FileSize
14.16 KB

All this time and I missed the mistake in the screenshots. This is the correct BEFORE screenshot, what is shown when a migration has already been performed.

masipila’s picture

quietone asked me in IRC to contribute on the help texts + handbook documentation.

I did read the IS but didn't go through all the comments. Is the concept in nutshell that:
1. Site builder runs the upgrade at time x

2. The old site remains the live PROD site while site builder finalizes the D8 site.
- No content should be created in D8 in order to avoid ID conflicts

3. When the configs of the D8 site are done, the site builder then runs the incremental migration in order to migrate the new content that was created after step 1.

Are rollbacks in the scope of this issue?

Cheers,
Markus

quietone’s picture

@masipila, I can't really say what different scenarios people would use the incremental migration for but when running from the UI there isn't a distinction between config/content, in that all the migrations will run. And this does not involve rollbacks, that is in #2687849: Add back rollbacks on migrate_drupal_ui.

phenaproxima’s picture

We discussed this in the weekly Migrate maintainer call. This issue is a release-blocking critical, so we shouldn't worry too much about fixing up the hook_help() text. We have a postponed issue to revisit the help text and spruce it up: #2939328: Migrate UI - review help text.

Therefore, why don't we remove this from the help text, since it is no longer true:

Incremental upgrades are not yet supported through the user interface.

...and make further improvements/rewrites in #2939328: Migrate UI - review help text.

Therefore, all we need is a change record, and that text to be removed, and then this is back to RTBC.

phenaproxima’s picture

We also discussed whether or not this still needs release manager review. We figure it doesn't, because the tag was originally added in the hopes of de-escalating this issue from a critical (see #76 and #81).

Obviously we didn't get to de-escalate it, so we think this no longer needs release manager review.

quietone’s picture

Status: Needs work » Needs review
FileSize
32.57 KB
1.21 KB

Removed text stating that incremental migration is not implement from the help text.

quietone’s picture

Issue tags: -Needs change record

Added change record but still not confident with making them. It definitely needs checking.

quietone’s picture

Issue summary: View changes

Update remaining tasks.

masipila’s picture

I updated the change record draft and added links to the relevant handbook pages in order to give context to the reader.

Note: if this patch will be cherry picked to 8.5.x (as I hope), the version in the change record needs to be updated from 8.6.x to 8.5.x.

Markus

masipila’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

All feedbak from #152 addressed. Remainingtasks updated in Issue Summary. RTBC.

quietone’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
32.57 KB
1.55 KB

Needed a reroll. The interdiff failed but a diff is readable.

Status: Needs review » Needs work

The last submitted patch, 159: 2687843-159.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
32.56 KB
1.81 KB

Ignore the previous patch. This should be better.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Gracias!

benjifisher’s picture

@leslieg and I did some wordsmithing on the change record.

quietone’s picture

Issue summary: View changes

Explain the changes to the test flow regarding aggregator.

Gábor Hojtsy’s picture

Adjusting credits.

  • Gábor Hojtsy committed 968c43f on 8.5.x
    Issue #2687843 by quietone, heddn, Jo Fitzgerald, alexpott, piggito,...

  • Gábor Hojtsy committed 669c2ef on 8.6.x
    Issue #2687843 by quietone, heddn, Jo Fitzgerald, alexpott, piggito,...
Gábor Hojtsy’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +SprintWeekend2018

Thanks everyone, this was/is a really important roadmap item for Migrate Drupal UI. I reviewed this issue over the weekend at Drupal Global Sprint Weekend and only found the aggregator changes confusing. That since has been explained to me and documented in the issue summary as well :)

Gábor Hojtsy’s picture

Slightly updated and published the change record at https://www.drupal.org/node/2939930

webchick’s picture

Issue tags: +8.5.0 highlights

Great work, everyone!

Status: Fixed » Closed (fixed)

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