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
Comment | File | Size | Author |
---|---|---|---|
#161 | diff-154-161.txt | 1.81 KB | quietone |
#161 | 2687843-161.patch | 32.56 KB | quietone |
#159 | diff.txt | 1.55 KB | quietone |
#159 | 2687843-159.patch | 32.57 KB | quietone |
#154 | interdiff.txt | 1.21 KB | quietone |
Comments
Comment #2
xjmComment #3
xjmComment #4
alexpottHere's a start - reintroduces incremental and extends the test coverage to do one.
Comment #6
alexpottFixing test
Comment #7
abhishek-anand CreditAttribution: abhishek-anand at Acquia commentedTested 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.
Comment #8
catchThe new fields are NULL after running updates:
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?
Comment #9
mikeryanI've tested this with a D7 site with custom blocks
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?
Comment #12
alexpottRather than an if in the form we should probably we using #access so the form array items are always present.
This looks really fragile. If migrate.fallback_state_key is not defined this will break.
Comment #13
mikeryanOn the weekly Migration hangout we decided this is not a critical - UI enhancements can go in at any time.
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedRerolled 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?
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedHmm, seem to have forgotten to upload the patch for 8.1.x
Comment #17
mallezieI'll look into the 8.2 test fails.
Comment #18
mallezieIf 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.
Comment #19
mallezieI think this should do the trick for the 8.2 patch.
Comment #20
quietone CreditAttribution: quietone as a volunteer commented@mallezie, thanks. I forgot forum was added.
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedRe 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?
Comment #22
xjmI'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. :)
Comment #23
benjy CreditAttribution: benjy at PreviousNext commentedI 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.
Comment #24
quietone CreditAttribution: quietone as a volunteer commentedAdded 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.
Comment #25
chx CreditAttribution: chx as a volunteer commentedHistorical 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.
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedThanks 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.
Comment #27
chx CreditAttribution: chx as a volunteer commentedThe 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.
Comment #28
mikeryanI'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...
Comment #29
quietone CreditAttribution: quietone as a volunteer commentedImplemented storing source_base_path in state. Works locally on a quick test with devel generated content.
Comment #30
mikeryanVery close now...
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.
Please flesh out the function docs.
Note that the current patch does not apply to 8.2.x.
Thanks!
Comment #31
quietone CreditAttribution: quietone as a volunteer commented1-2. Yes, of course. Both fixed.
Comment #33
quietone CreditAttribution: quietone as a volunteer commentedWell, thats what happens when you're not working from HEAD.
Comment #34
mikeryanTested 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)...
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedThe forum module is installed after the first migration, so the counts need to be adjusted to include both the forum and the aggregator modules
.
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedTests 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.
Comment #39
almaudoh CreditAttribution: almaudoh commentedBack to RTBC per #34 and #38.
Comment #41
xjmInteresting; #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!
Comment #43
xjmThose fails look suspicious. Re-RTBCing and retesting.
Comment #44
andypostComment #46
quietone CreditAttribution: quietone as a volunteer commentedThe tests are passing. Back to RTBC.
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedNo, tests are passing. Back to RTBC.
Comment #49
alexpottI 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.
Comment #50
mikeryanAlex 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:
#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
Comment #52
mikeryanRerunning tests, not sure that failure was relevant.
Comment #53
mikeryanTest passed, back to RTBC.
Comment #54
xjmAssigning to @alexpott based on his previous feedback for the issue.
Comment #55
xjmAs a feature addition for an alpha experimental module, this is rc eligible per: https://www.drupal.org/core/d8-allowed-changes#rc
Comment #56
alexpottThe 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.
Comment #57
alexpottComment #58
benjy CreditAttribution: benjy at PreviousNext commentedFor 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.
Comment #59
quietone CreditAttribution: quietone as a volunteer commentedFor 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.
Comment #60
mikeryanEssentially, 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.
Comment #61
xjmFor 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.
Comment #62
xjmWe 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.
Comment #63
alexpottI 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.
Comment #64
benjy CreditAttribution: benjy at PreviousNext commentedWas 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.
Comment #66
mikeryanComment #68
heddnThis is a blocker for #2905491: Mark Migrate Drupal UI as stable
Comment #69
webchickHm. 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.
Comment #70
webchickNote 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.
Comment #71
heddnI'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.
Comment #72
heddnBased 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.
Comment #73
webchickWell. Bear in mind that's a DRAFT. :) I think it's probably the release managers' call whether this is migrate critical or not.
Comment #74
heddnTagging
Comment #75
heddnComment #76
Gábor HojtsyIn #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).
Comment #77
webchickOk, adjusted the roadmap to bring the rollback functionality back under 8.5.0 targets: https://www.drupal.org/core/roadmap#migrate
Comment #78
quietone CreditAttribution: quietone as a volunteer commentedUpdated 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.
Comment #79
quietone CreditAttribution: quietone as a volunteer commentedAdded the issue this is postponed on to the IS.
Comment #80
heddnI 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.
Comment #81
heddnCan 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.
Comment #82
heddnstill needs tests, but this gets things moving.
Comment #84
heddnLet's make sure we use the correct branch, shall we?
Comment #85
quietone CreditAttribution: quietone as a volunteer commentedTwo minor fixes.
Comment #86
quietone CreditAttribution: quietone as a volunteer commentedAssigning to myself to add the tests.
Comment #87
quietone CreditAttribution: quietone as a volunteer commentedWhile 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.
Comment #88
quietone CreditAttribution: quietone as a volunteer commentedModified 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).
Comment #90
heddnIt seems like a legit error in the tests. I'm working to reproduce it. For now, here's some feedback.
Can we split this into separate "create" steps so we can call them individually or as a group?
Nit. and missing.
Comment #91
heddnThis is the issue with aggregator:
Comment #92
heddnOr 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.
Comment #94
heddnComment #96
quietone CreditAttribution: quietone as a volunteer commentedSince 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.
Comment #98
jofitz CreditAttribution: jofitz at ComputerMinds commentedFixed test failures: removed calls to (now non-existent)
createFeedContent()
because the code that was in there is now back increateContent()
, which is called on the next line.Fixed coding standards errors.
Comment #99
phenaproximaFairly shallow review, but I really like the extensive test coverage. Looks pretty good to me, overall.
Hah! :)
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.
Nit: setValue() is chainable, so we could do ->setValue()->setRebuild().
Can this doc comment be expanded a bit? "Setup migrations" tells me nothing more than the method name already does.
Nit: ->set() is chainable.
Thanks for this!
We should explain why we're doing this.
This should probably be pageTextContains().
Should end with "...after incremental migration".
$session needs a type hint (also in the method signature), and there's a typo in 'current'.
$session needs to be type hinted, and 'current' is misspelled. :)
'statics' should be 'static caches'.
Comment #100
quietone CreditAttribution: quietone as a volunteer commentedA 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?
Comment #101
heddnRe #100, Discussed in IRC with @phenaproxima and we're OK with punting those points of feedback until a follow-up.
Comment #102
heddnComment #103
quietone CreditAttribution: quietone as a volunteer commented1. 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
Comment #104
heddnComment #105
phenaproximaClose to godliness!
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.
Nit: These calls are chainable, so we can write them as $form_state->setValue()->setRebuild().
Same here.
Can we expand the doc comment to explain what this actually does?
All three parameters should be type hinted, in both the method signature and the doc comment.
$session should be type hinted in both spots.
'drupal' should be capitalized, and this should be an int, not a string, since it's a numeric value.
Nit: Can there be a blank line before $plugin_manager, for readability?
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?
Why don't we replace this if statement, and the pass() and fail() calls, with a pair of assertions instead? Something like:
Comment #106
anpolimusComment #107
quietone CreditAttribution: quietone as a volunteer commented@anpolimus, are you working on this? Its just that I have time now and can pick it up.
Comment #108
quietone CreditAttribution: quietone as a volunteer commented@anpolimus, sorry, I was being over zealous.
Comment #109
anpolimus@quietone I just want to make your comments fixed and prepare last version of the patch
Comment #110
anpolimusworking on it now
Comment #111
RytoEX CreditAttribution: RytoEX as a volunteer commentedNit: "verison" should be "version".
This caught my eye while reading #105 and wanted to write it down before I forgot.
Comment #112
Gábor HojtsyIs 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.
Comment #113
heddnUn-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.
Comment #114
benjifisherFirst, 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.
Comment #115
quietone CreditAttribution: quietone as a volunteer commentedWorking on the reroll.
Comment #116
quietone CreditAttribution: quietone as a volunteer commentedOnly the reroll. Interdiff failed too.
Comment #118
quietone CreditAttribution: quietone as a volunteer commentedTry again.
Comment #119
quietone CreditAttribution: quietone as a volunteer commentedReroll now working. Next is to address everything from #105 onwards.
Comment #120
quietone CreditAttribution: quietone as a volunteer commentedUpdates IS a bit, added before and after screenshots.
Comment #121
quietone CreditAttribution: quietone as a volunteer commentedEdit a sentence in the IS so it makes sense.
Comment #122
quietone CreditAttribution: quietone as a volunteer commentedRemove the postponed section from the IS. This was made active in #80.
Add update the tags.
Comment #123
quietone CreditAttribution: quietone as a volunteer commentedAnother step on the way. This fixes #105.2-10, and #111.
That just leaves #105.1
Comment #124
quietone CreditAttribution: quietone as a volunteer commentedComment #125
quietone CreditAttribution: quietone as a volunteer commentedThis should fix #105.1
Comment #126
phenaproximaI'm not seeing much to complain about here!
Nit: Variable names inside methods should be $snake_case, not $camelCase.
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();
What if user 2 already exists? I don't know if we should be setting the uid explicitly here.
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.
Comment #127
heddnLet'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?
Comment #129
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #130
jofitz CreditAttribution: jofitz at ComputerMinds commentedgetStorage()
.use
statements.Comment #132
jofitz CreditAttribution: jofitz at ComputerMinds commentedDunno how I missed those ones too!
Comment #133
benjifisherThe IS still needs some work:
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.
Comment #134
phenaproximaOnly one thing, then I think I'm happy with it.
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.
Comment #135
piggito CreditAttribution: piggito as a volunteer and at Skilld commentedFixing the observation on #134
Comment #136
piggito CreditAttribution: piggito as a volunteer and at Skilld commentedUploaded interdiff with wrong extension by mistake so reuploading it.
Comment #138
jofitz CreditAttribution: jofitz at ComputerMinds commentedIgnore test failure - that was just the interdiff.
Comment #139
phenaproximaAnd it's green. Therefore, let it be greener. Thanks all!
Comment #140
benjifisherI 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?
Comment #141
phenaproximaWhich part of the patch are you referring to, specifically?
Agreed about the IS update; kicking back to review for that.
Comment #142
phenaproximaHaving 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!
Comment #143
benjifisher@phenaproxima: I was thinking of your comment in #134:
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.
Comment #144
quietone CreditAttribution: quietone as a volunteer commentedAdded the new text to the IS as requested in #133.
Comment #145
benjifisherIs this still accurate?
Comment #146
quietone CreditAttribution: quietone as a volunteer commented@benjifisher, thanks
Updated the remaining tasks. Can the documentation be done in a follow up or does it need to happen here before commit?
Comment #147
benjifisherUnder "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:
*.api.php
if the patch adds new hooks (does not apply here, does it?)hook_help()
implementationHas
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.Comment #148
quietone CreditAttribution: quietone as a volunteer commentedThe
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.
Comment #149
quietone CreditAttribution: quietone as a volunteer commentedAll 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.
Comment #150
masipila CreditAttribution: masipila as a volunteer commentedquietone 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
Comment #151
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #152
phenaproximaWe 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:
...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.
Comment #153
phenaproximaWe 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.
Comment #154
quietone CreditAttribution: quietone as a volunteer commentedRemoved text stating that incremental migration is not implement from the help text.
Comment #155
quietone CreditAttribution: quietone as a volunteer commentedAdded change record but still not confident with making them. It definitely needs checking.
Comment #156
quietone CreditAttribution: quietone as a volunteer commentedUpdate remaining tasks.
Comment #157
masipila CreditAttribution: masipila as a volunteer commentedI 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
Comment #158
masipila CreditAttribution: masipila as a volunteer commentedAll feedbak from #152 addressed. Remainingtasks updated in Issue Summary. RTBC.
Comment #159
quietone CreditAttribution: quietone as a volunteer commentedNeeded a reroll. The interdiff failed but a diff is readable.
Comment #161
quietone CreditAttribution: quietone as a volunteer commentedIgnore the previous patch. This should be better.
Comment #162
phenaproximaGracias!
Comment #163
benjifisher@leslieg and I did some wordsmithing on the change record.
Comment #164
quietone CreditAttribution: quietone as a volunteer commentedExplain the changes to the test flow regarding aggregator.
Comment #165
Gábor HojtsyAdjusting credits.
Comment #168
Gábor HojtsyThanks 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 :)
Comment #169
Gábor HojtsySlightly updated and published the change record at https://www.drupal.org/node/2939930
Comment #170
webchickGreat work, everyone!