See #2748609: [meta] Preserving auto-increment IDs on migration is fragile.

Problem/Motivation

Preserving serial IDs in the upgrade process (e.g., source site nid 53 => destination site nid 53) means that if any content with the same ID is manually created on the destination side before upgrade, it will be overwritten with data from the source. Since the core upgrade process does not provide an option to not preserve IDs, we should warn the user before executing the upgrade process of any potential conflicts.

Proposed resolution

Run a process before the actual import which identifies conflict risks - cases where an ID in the content to be imported matches an ID that already exists in the destination.

  1. Maintain data integrity
    If this is a required step, the user can choose whether to proceed and overwrite any conflicts, or take an alternative approach (per the docs).
  2. No surprises
    They are told exactly what content could be affected by conflicts.
  3. Provide a path forward
    Yes.
  4. Preserve URLs (e.g., node/17).
    Yes.
  5. Minimize technical debt.
    Audit code should be reasonably isolated and maintainable.
  6. Minimize effort to implement.
    We’d need to first do what the import process normally does, identify and instantiate the necessary migrations. For each migration, we need to identify the destination ID field, see if that field is mapped in the process section, look for any of destination ID values which are not in the migration’s map table, and see if any of *those* IDs are in the source. Oh, but we need to consolidate this across all migrations to the same entity type… Ugh.

Remaining tasks

  • Code it
  • Architectural Review
  • Usability Review
  • Change record

User interface changes

The upgrade summary page (after entering db credentials, before executing migrations) should run the audit and display any conflicts.

API changes

EntityContentBase destination plugin and Sql Id map plugin are now implementing HighestIdInterface.

New classes:

  • AuditorInterface
  • IdAuditor
  • AuditResult
  • AuditException
  • HighestIdInterface

Data model changes

None.

CommentFileSizeAuthor
#257 interdiff-2876085-254-257.txt565 bytesphenaproxima
#257 2876085-257.patch62.87 KBphenaproxima
#254 interdiff-2876085-245-254.txt1.33 KBphenaproxima
#254 2876085-254.patch62.75 KBphenaproxima
#251 Selection_011.png49.46 KBquietone
#245 interdiff_244-245.txt1.49 KBheddn
#245 2876085-245.patch62.48 KBheddn
#244 interdiff-2876085-242-244.txt2.14 KBmaxocub
#244 2876085-244.patch62.71 KBmaxocub
#242 interdiff-2876085-240-242.txt1.1 KBmaxocub
#242 2876085-242.patch62.66 KBmaxocub
#242 conflicts.png72.82 KBmaxocub
#240 interdiff-2876085-238-240.txt3.38 KBmaxocub
#240 2876085-240.patch62.64 KBmaxocub
#240 conflicts.png73.52 KBmaxocub
#239 migrate-report.png204.97 KByoroy
#238 interdiff-2876085-235-238.txt4.47 KBmaxocub
#238 2876085-238.patch62.39 KBmaxocub
#235 2876085-235.patch62.31 KBheddn
#235 interdiff_222_235.txt3.92 KBheddn
#235 Screenshot_2017-12-12_12-55-07.png47.03 KBheddn
#226 Selection_010.png48.58 KBquietone
#222 interdiff-2876085-217-222.txt5.56 KBmaxocub
#222 2876085-222.patch62.64 KBmaxocub
#217 interdiff-2876085-210-216.txt7.38 KBmaxocub
#217 2876085-216.patch62.57 KBmaxocub
#211 interdiff-2876085-207-210.txt7.83 KBmaxocub
#211 2876085-210.patch58.83 KBmaxocub
#211 conflicts.png72.6 KBmaxocub
#210 Selection_008.png21.88 KBquietone
#207 interdiff-2876085-206-207.txt2.84 KBmaxocub
#207 2876085-207.patch55.56 KBmaxocub
#206 interdiff-2876085-203-206.txt4.25 KBmaxocub
#206 2876085-206.patch55.54 KBmaxocub
#203 interdiff-2876085-200-203.txt6.09 KBmaxocub
#203 2876085-203.patch55.22 KBmaxocub
#200 interdiff-2876085-197-200.txt3.78 KBmaxocub
#200 2876085-200.patch55.22 KBmaxocub
#197 interdiff-2876085-196-197.txt5.09 KBmaxocub
#197 2876085-197.patch55.26 KBmaxocub
#196 2876085-196.patch51.63 KBJo Fitzgerald
#196 interdiff-194-196.txt742 bytesJo Fitzgerald
#194 2876085-194.patch51.8 KBmaxocub
#194 interdiff-2876085-193-194.txt2.47 KBmaxocub
#194 screenshot.png17.08 KBmaxocub
#193 191-193.txt1.49 KBheddn
#193 191-193.txt1.49 KBheddn
#192 screenshot.png18.91 KBmaxocub
#191 interdiff-2876085-184-191.txt7.26 KBphenaproxima
#191 2876085-191.patch50.65 KBphenaproxima
#190 screenshot.png21.48 KBmaxocub
#184 interdiff-2876085-181-184.txt1.78 KBphenaproxima
#184 2876085-184.patch50.89 KBphenaproxima
#181 interdiff-2876085-179-181.txt8.67 KBphenaproxima
#181 2876085-181.patch50.87 KBphenaproxima
#179 interdiff-2876085-176-179.txt11.03 KBphenaproxima
#179 2876085-179.patch45.47 KBphenaproxima
#176 interdiff-2876085-174-176.txt1.42 KBmaxocub
#176 2876085-176.patch42.9 KBmaxocub
#174 2876085-174.patch42.95 KBmaxocub
#168 Selection_001.png34.31 KBquietone
#166 interdiff-2876085-162-166.txt6.91 KBmaxocub
#166 2876085-166.patch43.58 KBmaxocub
#162 2876085-162.patch47.58 KBheddn
#162 interdiff_156_162.txt2.23 KBheddn
#156 interdiff-2876085-154-156.txt4.51 KBmaxocub
#156 2876085-156.patch47.88 KBmaxocub
#154 2876085-154.patch44.28 KBheddn
#154 interdiff_149-154.txt20.08 KBheddn
#150 2876085-149.patch42.94 KBmaxocub
#146 interdiff-2876085-143-146.txt1.07 KBphenaproxima
#146 2876085-146.patch43.8 KBphenaproxima
#143 2876085-143.patch43.89 KBJo Fitzgerald
#143 interdiff-141-143.txt8.71 KBJo Fitzgerald
#141 2876085-141.patch43.82 KBmaxocub
#136 interdiff-2876085-130-136.txt8.34 KBphenaproxima
#136 2876085-136.patch43.8 KBphenaproxima
#130 interdiff-2876085-129-130.txt4.19 KBphenaproxima
#130 2876085-130.patch48.28 KBphenaproxima
#129 2876085-129.patch46.5 KBphenaproxima
#121 interdiff_119-121.txt3.08 KBheddn
#121 2876085-121.patch46.33 KBheddn
#119 interdiff_117-119.txt517 bytesheddn
#119 2876085-119.patch47.36 KBheddn
#117 interdiff_113-117.txt3.44 KBheddn
#117 2876085-117.patch47.37 KBheddn
#113 2876085-113.patch45.64 KBheddn
#113 interdiff_110-113.txt1.38 KBheddn
#110 2876085-110.patch44.56 KBJo Fitzgerald
#110 interdiff-107-110.txt1.73 KBJo Fitzgerald
#107 2876085-107.patch44.81 KBphenaproxima
#105 Screenshot_2017-09-01_09-30-11.png64.63 KBheddn
#105 interdiff_104-105.txt1.08 KBheddn
#105 2876085-105.patch50.85 KBheddn
#104 interdiff_97-104.txt1.48 KBheddn
#104 2876085-104.patch50.85 KBheddn
#104 Screenshot_2017-09-01_09-24-09.png64.63 KBheddn
#103 Screenshot_2017-09-01_09-15-35.png66.4 KBheddn
#101 Selection_007.png20.3 KBquietone
#99 Selection_005.png7.8 KBquietone
#97 interdiff_95-97.txt678 bytesheddn
#97 2876085-97.patch50.93 KBheddn
#95 interdiff_92-95.txt10.98 KBheddn
#95 2876085-95.patch50.94 KBheddn
#94 Selection_004.png5.76 KBquietone
#92 interdiff_90-92.txt7.26 KBheddn
#92 2876085-92.patch50.19 KBheddn
#92 Screenshot_2017-08-30_16-59-43.png65.41 KBheddn
#91 interdiff_89-90.txt7.25 KBheddn
#91 2876085-90.patch47.28 KBheddn
#89 interdiff_81-89.txt1.89 KBheddn
#89 2876085-89.patch45.43 KBheddn
#81 interdiff_75-81.txt3.88 KBheddn
#81 2876085-81.patch45.43 KBheddn
#78 2876085-78.patch45.4 KBheddn
#77 2876085-77.patch45.28 KBheddn
#75 interdiff_73-75.txt3.92 KBheddn
#75 2876085-75.patch44.51 KBheddn
#73 review.txt32.81 KBheddn
#73 interdiff_69-73.txt1.99 KBheddn
#73 2876085.73.patch43.19 KBheddn
#69 2876085-69.patch42.19 KBheddn
#68 2876085-68.patch25.7 KBheddn
#65 interdiff-2876085-63-65.txt23.49 KBmaxocub
#65 2876085-65.patch42.01 KBmaxocub
#63 review.txt32.35 KBmaxocub
#63 interdiff-2876085-62-63.txt13.11 KBmaxocub
#63 2876085-63.patch53.33 KBmaxocub
#62 interdiff-60-62.txt616 bytesmaxocub
#62 2876085-62.patch44.47 KBmaxocub
#60 review.txt23.68 KBmaxocub
#60 interdiff-58-60.txt32.88 KBmaxocub
#60 2876085-60.patch44.47 KBmaxocub
#58 interdiff-55-58.txt4.49 KBmaxocub
#58 2876085-58.patch52.7 KBmaxocub
#55 2876085-55.patch50.63 KBheddn
#55 interdiff_53-55.txt1.3 KBheddn
#53 review.txt28.92 KBmaxocub
#53 interdiff-50-53.txt5.09 KBmaxocub
#53 2876085-53.patch50.26 KBmaxocub
#50 review.txt25.6 KBmaxocub
#50 interdiff-48-50.txt5.19 KBmaxocub
#50 2876085-50.patch46.52 KBmaxocub
#48 review.txt23.14 KBheddn
#48 interdiff_44-48.txt11.14 KBheddn
#48 2876085-48.patch43.76 KBheddn
#44 Selection_173.png41.09 KBheddn
#44 review.txt21.56 KBheddn
#44 2876085-44.patch42.01 KBheddn
#44 interdiff_36-44.txt3.89 KBheddn
#40 interdiff-36-40.txt3.23 KBmaxocub
#40 2876085-38.patch42.27 KBmaxocub
#38 interdiff-36-38.txt3.23 KBmaxocub
#38 2876085-38.patch42.28 KBmaxocub
#36 interdiff_35-36.txt7.13 KBheddn
#36 2876085-36.patch41.56 KBheddn
#35 2876085-35.patch34.57 KBheddn
#35 interdiff_24-35.txt61.59 KBheddn
#24 interdiff_22-24.txt791 bytesheddn
#24 2876085.patch96.16 KBheddn
#22 2876085-22.patch95.39 KBheddn
#22 review.txt14.1 KBheddn
#22 interdiff_19-22.txt1.32 KBheddn
#19 2876085-18.patch95.4 KBheddn
#17 2876085-17.patch97.57 KBheddn
#17 interdiff_13-17.txt84.12 KBheddn
#15 Selection_012.png21.85 KBquietone
#13 2876085-13.patch15.51 KBheddn
#13 interdiff_7-13.txt3.99 KBheddn
#7 2876085-6.patch15.55 KBvasi
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mikeryan created an issue. See original summary.

vasi’s picture

At the triage, I discussed the possibility of making this much simpler:

The current approach requires looking at all of the source, process and destination in complex ways, to identify exactly which new IDs will be migrated.

Instead, we could simply check "are there any new destination entities of certain types since the last migration (if any)?" If so, warn the user that these new entities might be overwritten.

This risks false positives: It could be that you have created nid 5, and are about to migrate nids 3, 4 and 6. Even though the migration would work, the simple check will warn you. But this case sounds quite rare, and careful users who know they fall in this case can click through the warning.

cilefen’s picture

Priority: Critical » Major
Issue tags: +Triaged D8 major

@xjm, @alexpott, @effulgentsia, @lauriii, @catch and I discussed this issue at a recent meeting and agreed that since the parent is critical, we would downgrade this to major but also make the other related children major.

heddn’s picture

We discussed this in the weekly migrate meeting and mikeryan, quiteone and heddn all agreed with vasi's approach as outlined in #2.

vasi’s picture

Ok, so here's the proposed implementation plan:

  • Create an interface with the getHighestId() method
  • Implement this on id_map, which will yield the highest ID that has been migrated.
  • Implement this on content destinations, which will yield the highest ID that exists of the destination type. Eg: highest node ID on the site.
  • Add a new field 'stable_ids' field to content destination configuration. If this is set and true, on the first import they will check that </code>$id_map->getHighestId() == $destination->getHighestId(). If not, throw an exception. (Maybe we want to do this as a global pre-flight check instead, so as to not waste user time?)
  • Add the stable_ids to all the relevant migration definitions. Eg: Add to d6_node, but not d6_node_translation, since it doesn't use an auto-increment ID. It's too annoying to automatically determine whether a migration is intended to have stable IDs, so I think this is the best option.
mikeryan’s picture

  • Create an interface with the getHighestId() method
  • Implement this on id_map, which will yield the highest ID that has been migrated.
  • Implement this on content destinations, which will yield the highest ID that exists of the destination type. Eg: highest node ID on the site.

Good thought - and if/when we do #2876086: Manipulate auto-increment values to avoid conflicts, we'd implement this on sources as well, so we'd know how high to set the auto-increment value.

(Maybe we want to do this as a global pre-flight check instead, so as to not waste user time?)

Yes, that's the plan as specified in the issue summary here - this is a pre-flight check, runtime checks would be done in #2876090: Warn if migrated content will overwrite manually-created content.

vasi’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
15.55 KB

Here's a teensy implementation. Still needs tests, and vetting.

vasi’s picture

Status: Needs review » Needs work

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

vasi’s picture

Note also that the patch doesn't really do a good job of deciding when to audit IDs. We'll probably want to add 'preserve_ids: true' to all the relevant YAML files? Or we can try to use heuristics to do the equivalent automatically.

vasi’s picture

At migration triage, it was decided to prefer a top-level property for this. Also, the patch needs to be modified to handle multiple migrations to the same destination type (eg: nodes of different types).

heddn’s picture

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -289,4 +297,70 @@ protected function getDefinitionFromEntity($key) {
    +    return (int)reset($found);
    

    Nit.Space.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -289,4 +297,70 @@ protected function getDefinitionFromEntity($key) {
    +  protected function getHighestIdField() {
    +    return $this->getKey('id');
    

    Why is this needed? Should this be public and on the interface?

  3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -289,4 +297,70 @@ protected function getDefinitionFromEntity($key) {
    +    if (isset($this->configuration['preserves_ids'])
    +      && !$this->configuration['preserves_ids']
    

    Can we just check !empty()?

  4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
    @@ -78,4 +78,11 @@ public function getIds() {
    +  public function getHighestIdField() {
    +    return $this->getKey('revision');
    +  }
    

    Why is this public but ECB is protected?

  5. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -923,4 +924,22 @@ public function valid() {
    +    return (int)$found;
    

    Nit.Space.

  6. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -1034,6 +1061,64 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +    $form['actions']['submit']['#value'] = $this->t('I acknowledge I may lose data, continue anyway');
    

    Nit. Period at end of sentence.

heddn’s picture

FileSize
3.99 KB
15.51 KB
heddn’s picture

Nits in my review are now addressed and we're now looking at a top-level attribute. Next up is manual testing and modifying to handle multiple migrations to the same destination type (eg: nodes of different types).

Plus we'll need automated tests.

quietone’s picture

FileSize
21.85 KB

Just a wee manual test. Installed D8 using my script which will then enabled migrate modules and admin_toolbar. Then went to '/upgrade' and completed the form for a D7 site. The result was the warning message about entities may be overwritten. Yes, there are shortcuts but 'preserves_id' is false for the shortcut migration.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -289,4 +297,64 @@ protected function getDefinitionFromEntity($key) {
+    if (!empty($this->migration->getPluginDefinition()['preserves_ids'])) {

Maybe change to isset? And since ids is plural we should change preserves_ids to preserve_ids.


I also think the working needs improvement. But it is too late in the day to do anything about that.

maxocub’s picture

I would also suggest, and I think it was mentioned in a previous meeting, that we should use an 'opt in' tag instead of an 'opt out'. Something like audit_ids: true to audit ids instead of preserves_ids: false to not audit ids.

heddn’s picture

Status: Needs work » Needs review
FileSize
84.12 KB
97.57 KB

What a bummer. That bumps the diff from 15k to 97k. We'll have to work on a review only patch from here on out.

maxocub’s picture

About the multiple migrations to the same destination type (eg: nodes of different types), I don't understand what needs to be done.

If there's an id conflict with migrated nodes, does it makes a difference if it's an article or a page? All of them are stored in the same table and have the same id field.

heddn’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -332,22 +332,18 @@ public function getHighestIdField() {
-    if (!empty($this->migration->getPluginDefinition()['preserves_ids'])) {
-      // If IDs are not preserved, we can't have conflicts.
-      return FALSE;
+    // If IDs are are audited, we can't have conflicts.
+    if (!empty($this->migration->getPluginDefinition()['audit_ids'])) {
+      if (!($idMap instanceof MigrateMaxIdInterface)) {
+        // We don't know how to audit IDs without a cooperating ID map.
+        return FALSE;
+      }
+      $highestMigrated = $idMap->getMaxId($this->getHighestIdField());
+      if ($this->highestDestinationId() > $highestMigrated) {
+        // There's a new ID that we might conflict with!
+        return TRUE;
+      }
     }
-
-    if (!($idMap instanceof MigrateMaxIdInterface)) {
-      // We don't know how to audit IDs without a cooperating ID map.
-      return FALSE;
-    }
-
-    $highestMigrated = $idMap->getMaxId($this->getHighestIdField());
-    if ($this->highestDestinationId() > $highestMigrated) {
-      // There's a new ID that we might conflict with!
-      return TRUE;
-    }
-    return FALSE;
   }

For reviewers, above are the relevant changes. The rest are all yaml changes in #17/#18. #18 is a new patch where I backed out the audit_ids config on some test assets:
core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/migrate.migration.external_translated_test_node.yml
core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/migrate.migration.external_translated_test_node_translation.yml
core/modules/taxonomy/tests/modules/taxonomy_term_stub_test/migrations/taxonomy_term_stub_test.yml

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

Status: Needs review » Needs work

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

heddn’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
14.1 KB
95.39 KB

To generate the review.txt, I used the follow:

$ git diff --cached >2876085-22.patch
$ git diff --cached -G "audit_ids: true" > match.txt
$ interdiff match.txt 2876085-22.patch  >review.txt

I also agree with #18. We don't care about the types. Probably up next is to fix the UI tests. What's after that?

Status: Needs review » Needs work

The last submitted patch, 22: 2876085-22.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
96.16 KB
791 bytes
quietone’s picture

Why have 'audit_ids: ' defined in all migrations? Surely we can have a default? Will it be the expectation that custom migrations add it? Can we use 'no_audit_ids: true' when we don't want to audit?

maxocub’s picture

I don't think it's necessary to put the audit_id tag everywhere. My understanding was that we were going to put it only in the migrations where we know it is needed.

If we don't want to audit IDs, we don't need to put audit_ids: false since this line evaluate as false if the tag is not present:

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -289,4 +297,60 @@ protected function getDefinitionFromEntity($key) {
+    if (!empty($this->migration->getPluginDefinition()['audit_ids'])) {
maxocub’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -289,4 +297,60 @@ protected function getDefinitionFromEntity($key) {
+      if (!($idMap instanceof MigrateMaxIdInterface)) {
+        // We don't know how to audit IDs without a cooperating ID map.
+        return FALSE;

For now, only EntityContentBase is an instance of MigrateIDAuditInterface, so only migrations with a destination extending EntityContentBase should use audit_ids (if needed). All other migrations that are not content will never have there IDs audited, so the audit_ids tag is really not necessary there.

Also, I tested the patch manually with the D7 fixtures and I found that if you enable the Forum module you will have a taxonomy term (General discussion) created on the destination site, and so you will be presented with the conflict warning page. That is why this line was needed in the tests:

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
@@ -147,6 +147,7 @@ public function testMigrateUpgrade() {
+    $this->drupalPostForm(NULL, [], t('I acknowledge I may lose data, continue anyway.'));

Is this a problem? It might confuse people.

quietone’s picture

Is this a problem? It might confuse people.

Yes, the new screen needs work. Do we work on it some more ourselves or tag for Usability review?

vasi’s picture

Sorry I haven't had time to help with this lately!

Unless I'm missing something, the patch still needs to be rearranged to correctly handle multiple migrations with the same destination types. Eg:

  • User runs migrate_upgrade from a D7 site. Five new nodes of type 'page' are created, with nids 1-5
  • User run an incremental migrate_upgrade. No new nodes have been created, so this should succeed.
    • Migration 'node__page' runs 'unsafeIdsExist()'. The ID map says the highest nid migrated is 5, and the destination says the highest nid on the site is 5. Those are equal, so this migration says we're ok!
    • Migration 'node__article' runs 'unsafeIdsExist()`. The ID map says no nid has been migrated, and the destination says the highest nid on the site is 5. Since 5 > 0, the migration incorrectly says we're at risk of overwriting data.

    There are several potential fixes. I think the most resilient one would be to aggregate migrations with the same destination plugin ID—then compare $destination->highestDestinationId() with max(array_map($migrations, 'highestIdField')) (pseudocode). Sound reasonable?

    We probably also have to somehow handle revision IDs, ick.

heddn’s picture

We discussed this in the weekly migrate meeting today. The summary of the conversation: forum module actually introduces the situation that we are trying to resolve with this audit. It inserts a term into a new vocabulary with a term name of 'General Discussion', probably as term #1. This is the topic for the initial forum. If you migrate over from D6/D7 and you have term data (which you probably do) then it will overwrite term #1 with the data that was in D6. This results in your General Discussion term magically getting replaced as 'Cats' or whatever the term was in D6/D7.

To address this, we remembered we had initial thought that we would link out to a handbook page discussing some potential situations and solutions. Forum module is a bit of an edge case, it isn't as popular as some of the other modules. It might just be easiest to add it as a scenario to the docs.

But one thing we didn't know in the meeting was if Forum was unique here. So, let's do some additional testing to see if any other modules in core do similar things.

heddn’s picture

Next steps

  1. Add a handbook page to discuss this.
  2. Add link for handbook page to UI page.
  3. Automated test of an upgrade without forum module enabled.
  4. We don't need to add a test for with, because we already have one.
maxocub’s picture

Status: Needs review » Needs work

Back to NW

krystalcode’s picture

Status: Needs work » Needs review

Added a section at https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d..., based on my understanding of the situation. Feel free to make any corrections or additions.

heddn’s picture

re #27: I found a listing of entity things over in #2711099: Categorize migrations according to their type. Going to work on that and adding a test.

+            'd6_aggregator_feed',
+            'd7_aggregator_feed',
+            'd6_aggregator_item',
+            'd7_aggregator_item',
+            'd7_blocked_ips',
+            'd6_book',
+            'd6_comment',
+            'd7_comment',
+            'd6_custom_block',
+            'd7_custom_block',
+            'd6_file',
+            'd7_file',
+            'd6_upload',
+            'd6_menu_links',
+            'd7_menu_links',
+            'd6_profile_values',
+            'd7_shortcut',
+            'd7_shortcut_set_users',
+            'd6_taxonomy_term',
+            'd7_tracker_node',
+            'd7_tracker_user',
+            'd6_url_alias',
+            'd7_url_alias',
+            'd6_user',
+            'd7_user',
+            'd6_user_contact_settings',
+            'd6_user_picture_file',
heddn’s picture

FileSize
61.59 KB
34.57 KB

ok, this only templates from the list above. To reduce the list from *all* templates, I took the list from the related issue and did a quick search/replace to make a series of commands like: git reset HEAD core/*/action_settings.yml, etc.

Interdiff attached if you finding it interesting. Next up is tests.

heddn’s picture

FileSize
41.56 KB
7.13 KB

Status: Needs review » Needs work

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

maxocub’s picture

Status: Needs work » Needs review
FileSize
42.28 KB
3.23 KB

First attempt to handle multiple migrations to the same destination (d7_node:artice, d7_node:page).
Tests will follow.

Status: Needs review » Needs work

The last submitted patch, 38: 2876085-38.patch, failed testing. View results

maxocub’s picture

Status: Needs work » Needs review
FileSize
42.27 KB
3.23 KB

Oups

Status: Needs review » Needs work

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

maxocub’s picture

Since this will be a hard issue to review, would it be OK to concentrated on the code and tests here, with only a few audit_ids to test with, and to open a follow up to tag all migrations that needs auditing? It think it would make our job and the reviewer's job a lot easier.

heddn’s picture

@maxocub to create a review patch, see #22 for instructions.

Now a review of #40.

+++ b/core/modules/migrate/src/Plugin/MigrateMaxIdInterface.php
@@ -10,11 +10,14 @@
+  public function getMaxId($field, $bundles);

I see what we are doing here now. I was curious why this was needed. So bear with me while I verbalize my thoughts. If there is a page and page_custom content types in the source, there would be 2 derivative migrations on the destination aligned with those 2 sources. However, if someone built a custom migration and piped those two sources into the same page content type (because that's what they want them to be on their d8 site), then there will be problems. There would be 2 source bundles but only a single mapping table. Let's call this scenario #1.

Alternatively, some builds a completely custom solution from CSVs and has only a single source but multiple bundles on the destination... then we'd have multiple bundles but only a single destination map. Scenario #2.

Last example. A single page content type on the legacy site. Custom migration. The single source is getting split into 3 content types on the destination with 3 different migrations and ignoring (skipping) content that isn't destined for that content type. Perhaps its breaking things up by media type so videos go one place. Audio another direction and photos the last content type. Scenario #3.

All the above are valid scenarios and aren't even that uncommon. Let's explore why bundle is important now. I'm not sure it is.

In any scenario, if the highest nid, media id, term_id, etc is greater than any mapping table's, we'll throw a warning. It indicates that the destination entity has more entities created than which came from a migration. In many cases, this is fine. Especially if you aren't mapping ids from old system to new. But if you mapping entity ids, then it does become important.

heddn’s picture

Let's see if we can resolve the test failures. Interdiff and changes against #36 (for now). If we decide to, we can add in #40 at a later point.

And from a UX perspective, I've also grabbed a screenshot of the conflicts page. It now has a link off to the recent handbook page entry. Maybe not the prettiest, but I'd don't think its too ugly either. It serves the purpose for solving a critical. If we want to bikeshed much on it, let's fork that to a follow-up, please.

conflict id upgrade page

Status: Needs review » Needs work

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

maxocub’s picture

Quick review of last patch.

1.

+++ b/core/modules/migrate/src/Plugin/MigrateMaxIdInterface.php
@@ -0,0 +1,20 @@
+namespace Drupal\migrate\Plugin;
+
+
+interface MigrateMaxIdInterface {

Extra line.

2.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -289,4 +297,60 @@ protected function getDefinitionFromEntity($key) {
+  /**
+   * Check whether unsafe IDs exist that should inhibit migration.
+   *
+   * @param \Drupal\migrate\Plugin\MigrateIdMapInterface $idMap
+   *   The ID map for this migration.
+   *
+   * @return bool
+   *   Whether unsafe IDs exist.
+   */
+  public function unsafeIdsExist(MigrateIdMapInterface $idMap) {

Should use {@inheritdoc}.

3.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -289,4 +297,60 @@ protected function getDefinitionFromEntity($key) {
+    // If IDs are are audited, we can't have conflicts.
+    if (!empty($this->migration->getPluginDefinition()['audit_ids'])) {

Should be 'we can have conflicts".

4.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -289,4 +297,60 @@ protected function getDefinitionFromEntity($key) {
+   * @param \Drupal\migrate\Plugin\MigrateIdMapInterface $idMap
...
+  public function unsafeIdsExist(MigrateIdMapInterface $idMap) {
...
+      if (!($idMap instanceof MigrateMaxIdInterface)) {
...
+      $highestMigrated = $idMap->getMaxId($this->getHighestIdField());
+      if ($this->highestDestinationId() > $highestMigrated) {

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -923,4 +924,22 @@ public function valid() {
+    $sqlField = $this->destinationIdFields()[$field];
...
+      ->fields('map', [$sqlField])
+      ->orderBy($sqlField, 'DESC')

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -723,12 +739,18 @@ class MigrateUpgradeForm extends ConfirmFormBase {
+  public function __construct(StateInterface $state, DateFormatterInterface $date_formatter, RendererInterface $renderer, MigrationPluginManagerInterface $plugin_manager, EntityTypeManagerInterface $entityTypeManager, MigrateIdAuditor $idAuditor) {
...
+    $this->entityTypeManager = $entityTypeManager;
+    $this->idAuditor = $idAuditor;

@@ -1082,6 +1109,64 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
+    $migrationIds = array_keys($form_state->get('migrations'));
...
+    $typeIds = $this->idAuditor->auditIds($migrations);
...
+    sort($typeIds);
+    foreach ($typeIds as $typeId) {
+      $items[] = $this->entityTypeManager->getDefinition($typeId)->getLabel();

This is maybe too much of a nit, so fell free to ignore, but the coding standard says to use same types of variable names (snake_case / lowerCamelCase) inside a file.

5.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgradeConflicts6Test.php
@@ -0,0 +1,57 @@
+use Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase;
+use Drupal\user\Entity\User;

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgradeConflicts7Test.php
@@ -0,0 +1,57 @@
+use Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase;
+use Drupal\user\Entity\User;

Unused use statements.

6.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
@@ -15,6 +15,21 @@
   /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = [

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgradeConflicts6Test.php
@@ -0,0 +1,57 @@
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   *
+   * Forum module adds content, resulting in content id conflicts.
+   */
+  public static $modules = [

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgradeConflicts7Test.php
@@ -0,0 +1,57 @@
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   *
+   * Forum module adds content, resulting in content id conflicts.
+   */
+  public static $modules = [

Should use {@inheritdoc}

7.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgradeConflicts6Test.php
@@ -0,0 +1,57 @@
+    // Ensure submitting the form with invalid database credentials gives us a
+    // nice warning.
+    $this->drupalPostForm(NULL, [$driver . '[database]' => 'wrong'] + $edits, t('Review upgrade'));
+    $this->assertText('Resolve the issue below to continue the upgrade.');

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgradeConflicts7Test.php
@@ -0,0 +1,57 @@
+    // Ensure submitting the form with invalid database credentials gives us a
+    // nice warning.
+    $this->drupalPostForm(NULL, [$driver . '[database]' => 'wrong'] + $edits, t('Review upgrade'));
+    $this->assertText('Resolve the issue below to continue the upgrade.');

This is already tested in the parent class.

heddn’s picture

Re #38 & @maxocub: I think I understand the problem now. To solve, I think we need to inspect all the migration destinations and for all that "match" the entity type, see what they have recorded in their mapping table. We loop over all migrations in #2711099: Categorize migrations according to their type, so a similar approach is what I'd expect here.

Next up, I'll work in responses to #46. I'm leaving #38 /
#22 for a later point. Maybe even by someone else.

heddn’s picture

Status: Needs work » Needs review
FileSize
43.76 KB
11.14 KB
23.14 KB

Setting to NR for test bot, but we still need to address #22.

Status: Needs review » Needs work

The last submitted patch, 48: 2876085-48.patch, failed testing. View results

maxocub’s picture

Status: Needs work » Needs review
FileSize
46.52 KB
5.19 KB
25.6 KB

Here's a second POC for handling multiple migrations to the same destination. What do you think? (Still needs tests)

I also changed back the assertSame to assertEquals to make the migrate_drupal_ui tests pass.

Status: Needs review » Needs work

The last submitted patch, 50: 2876085-50.patch, failed testing. View results

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
50.26 KB
5.09 KB
28.92 KB

Here's fixing the Unit tests.

@heddn: I'm not sure I like the idea of extending MigrateUpgrade6Test & MigrateUpgrade7Test for the conflicts test. Those are so long to run and they often need to be updated when we modify the fixtures. If we extend them, it means two times as long to run and two more places to update the entity count.
I did spent a lot of time trying to make them pass, and when I finally got the counts right, there was this block migration that was failing due to the forum module not being enabled.
If you're OK with that, I'm going to try to work on dedicated tests.

Status: Needs review » Needs work

The last submitted patch, 53: 2876085-53.patch, failed testing. View results

heddn’s picture

re #53 we might have to do that, but then we'd be dropping a "test all the things" test case, more or less. Let's try a little longer to see if we can make things pass. Or if you have an idea to keep the "test all the things" and a non-conflicts test case, that might be another option.

maxocub’s picture

Sorry that I didn't uploaded my patch with the updated counts, but it's not just the field_storage_config that needs updating, but also field_config, taxonomy_vocabulary, rdf_mapping, base_field_override (and maybe another one, I don't remember) and you have to lower it in the test where Forum is not enabled and keep the initial value in the test where Forum is enabled. ;)

If we keep the "test all the things" scenario, we could just test that there is conflicts, but not migrate and count entities, and in other tests, do the migration and entity count but on selected migrations?

Status: Needs review » Needs work

The last submitted patch, 55: 2876085-55.patch, failed testing. View results

maxocub’s picture

Status: Needs work » Needs review
FileSize
52.7 KB
4.49 KB

Here's the correct entity counts.
There should now be only one test failing (MigrateUprade6Test) with this error:

1) Drupal\Tests\migrate_drupal_ui\Functional\d6\MigrateUpgrade6Test::testMigrateUpgrade
Migration of a:3:{s:9:"sourceid1";s:5:"forum";s:9:"sourceid2";s:1:"0";s:9:"sourceid3";s:7:"garland";} to a:0:{} as part of the d6_block migration. The source row status is 3

Maybe it's a problem with the bloc migration, but it's unable to migrate the forum blocs if forum is not enabled.

Status: Needs review » Needs work

The last submitted patch, 58: 2876085-58.patch, failed testing. View results

maxocub’s picture

Status: Needs work » Needs review
FileSize
44.47 KB
32.88 KB
23.68 KB

I started to work on tests, using KTB instead of the slow BTB. This is just a begining, I plan on adding more of them.
The interdiff is a bit bloated because I did a little refactoring of the API to make it cleaner, I think.

Status: Needs review » Needs work

The last submitted patch, 60: 2876085-60.patch, failed testing. View results

maxocub’s picture

Oups, wrong class name.

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
53.33 KB
13.11 KB
32.35 KB

I added tests auditing the obvious entity migrations for D6 & D7. I'll add more tests for the other places where we need to audit IDs.

heddn’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record
+++ b/core/modules/migrate/src/Plugin/MigrateDestinationAuditInterface.php
@@ -0,0 +1,35 @@
+  public function getEntityType();

We should remove this helper function from the interface so we can have this solution work with non-entities as well. Plus, we should look at i18n and revisions.

maxocub’s picture

I removed the getEntityType() method from the interface and moved things a bit. Now the ID auditor is using the migration's base_id & label for it's report, instead of the entity's id & label, since we want to support more than just entities.

Let's start by auditing IDs of the obvious entity types and then move on to the revisions, translations and other exceptions.

heddn’s picture

@catch, if we fix this for everything but i18n and revisions but can prove that this solution in the API will work, can we mark the API module stable? Or is the API module's stability dependent on completely solving this for those two pieces as well. I'm hoping we can prove it *can* solve the problem for revisions, but leave the solving of the problem to migrate drupal. And then mark the API module stable a little bit earlier.

heddn’s picture

Issue tags: +Needs re-roll
heddn’s picture

FileSize
42.19 KB

Missed some hunks on the re-roll.

heddn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

webchick’s picture

Issue tags: +Migrate API

Inventing a new tag to illustrate that this is a Migrate critical specifically blocking Migrate API's stability. (Please advise if this is wrong.)

heddn’s picture

Status: Needs work » Needs review
FileSize
43.19 KB
1.99 KB
32.81 KB

Let's see if that fixes the test failures.

Status: Needs review » Needs work

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

heddn’s picture

Status: Needs work » Needs review
FileSize
44.51 KB
3.92 KB

Added initial support for revision. One question I have for i18n, do we need to make the audit API support more than a single "key". I have a feeling we might, but perhaps someone else can confirm?

Status: Needs review » Needs work

The last submitted patch, 75: 2876085-75.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
45.28 KB
heddn’s picture

Sorry for all the noise. Not sure why the tests are failing. And I cannot reproduce it locally.

The last submitted patch, 77: 2876085-77.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 78: 2876085-78.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
45.43 KB
3.88 KB

This should fix the tests. Interdiff attached that goes back to #75.

'Entities may be overwritten' != 'Entities may be overwritten?'

Status: Needs review » Needs work

The last submitted patch, 81: 2876085-81.patch, failed testing. View results

maxocub’s picture

Re #75

One question I have for i18n, do we need to make the audit API support more than a single "key". I have a feeling we might, but perhaps someone else can confirm?

Since translating a node does not create a new node, the highest ID does not increase. So if we only support one key (the nid) the audit won't show any conflicts in this scenario:

1. You migrate d7_node first
2. You add translations manually (no new nids, only new langcodes)
3. You migrate d7_node_translation. There should be conflicts even though there's no new nids.

So I guess only supporting one key (the nid) is not sufficient, but how can we find the highest destination ID when we are dealing with langcodes, that I really don't know.

heddn’s picture

The other thing to consider is that paragraphs uses a composite identifier of an id and its revision. I'm not sure if the revision is sequential and you can "just" use it, or if we need to handle the both.

heddn’s picture

For i18n, we can probably check if the destination supports translation and use that as part of the conflict identification.

  1. +++ b/core/modules/migrate/src/MigrateIdAuditor.php
    @@ -0,0 +1,60 @@
    +        $destination = $migration->getDestinationPlugin();
    ...
    +          if ($destination->getHighestDestinationId($field_name) > $id_map->getHighestMigratedId($field_name)) {
    

    Maybe here we just need to check if translations are enabled on the destination and then check if en or es, etc was already inserted into the destination. basically filter getHighestMigratedId by langcode.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +940,48 @@ public function valid() {
    +        return 0;
    

    I wonder if this should be a break instead of a return. If a single mapping table doesn't exist, it doesn't mean that other mapping tables don't.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +940,48 @@ public function valid() {
    +      $query = $this->getDatabase()->select($map_table, 'map')
    +        ->fields('map', [$sql_field])
    +        ->orderBy($sql_field, 'DESC')
    +        ->range(0, 1);
    +      $ids[] = $query->execute()->fetchField();
    

    Potentially we could filter by langcode. Which would change the interface of this function.

heddn’s picture

Checked with Berdir and paragraphs, while it uses a composite id, the revision_id is sequential. This answers my question from #84 that getAuditedIdFieldName can just be a single field name's value. We just need to decide if getHighestDestinationId should key its values by langcode.

heddn’s picture

I've opened #2905743: Add UI components of audit/conflicts to drush runner to create the drush equivalent to migrate_drupal_ui's execution of the audit.

heddn’s picture

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

The tests came back green on local. Fingers crossed.

Status: Needs review » Needs work

The last submitted patch, 89: 2876085-89.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
47.28 KB
7.25 KB

I think we've figured out how to handle revisions in this patch. I could use someone else's eyes to confirm that.

What's remaining is the i18n/translations. That one is proving hard. Per #83, we might have to solve it entirely differently. I'm proposing a very ugly solution, but it would at least keep us moving.

If the destination is translatable, then just warn the user that it *could* result in issues. Link to a d.o. page that discusses some of the technicals. If a site has i18n on it, I cannot think of a scenario where there isn't going to be some custom migration work anyway. So warn and work on fixing it with follow-ups. And for the rest of the world who isn't i18n, they won't even know about this scary part of the Drupal universe.

heddn’s picture

Version: 8.5.x-dev » 8.4.x-dev
Issue summary: View changes
FileSize
65.41 KB
50.19 KB
7.26 KB

I've opened a follow-up for the i18n. I haven't currently marked it migrate critical, since I'm hoping that we can just handle i18n translations with documentation, as I'm doing in the attached patch. Tests will need some work as this new approach is untested. Looking for feedback.

Follow-up: #2905759: Before upgrading, audit for potential i18n ID conflicts

Screenshot of the new conflicts page:

quietone’s picture

Status: Needs review » Needs work

@heddn, @maxocub, @vasi, Awesome work!

Had a read through and found a bunch of nits. There are two code questions, the first item and then another about which assertion to use.

  1. +++ b/core/modules/migrate/src/MigrateIdAuditor.php
    @@ -0,0 +1,60 @@
    +    if (is_string($label)) {
    +      $conflict[$base_id] = $label;
    +    }
    +    elseif ($label instanceof TranslatableMarkup) {
    

    If there is no label nothing will be recorded? I know we check for the existence of labels in core, but if someone adds audit to their migration without a label, there will be no notification.

  2. +++ b/core/modules/migrate/src/MigrateIdAuditor.php
    @@ -0,0 +1,60 @@
    +
    

    Remove line to be consistent with previous method.

  3. +++ b/core/modules/migrate/src/MigrateIdAuditorInterface.php
    @@ -0,0 +1,28 @@
    +   * system knowing about it. To find out if there is ID conflicts, this method
    

    Remove extra line

  4. +++ b/core/modules/migrate/src/MigrateIdAuditorInterface.php
    @@ -0,0 +1,28 @@
    +   * the former is greater than the later, there is ID conflicts.
    

    s/later/latter/ and s/is/are/

  5. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationAuditInterface.php
    @@ -0,0 +1,42 @@
    +   * Get whether this destination is for translations.
    

    s/Get/Gets/

  6. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapAuditInterface.php
    @@ -0,0 +1,23 @@
    + * Should be implemented by id_map plugins that are able to audit their
    

    Isn't this supposed to have a one line summary followed by a blank line and the a more detailed description?
    The use of 'should' here doesn't make it clear, to me, what to do. My first thought, was what happens if I don't?

  7. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapAuditInterface.php
    @@ -0,0 +1,23 @@
    +   *   The name of the field containing the IDs that should be audited.
    

    s/should be/will be/

  8. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -17,8 +18,15 @@
    + *     means that, for the entity type this migration creates, if the highest
    

    Having the doc here can make one believe that audit_ids is a configuration on the destination, when it is the migration. What is a better place?

  9. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -17,8 +18,15 @@
    + *     be trown that entities might be overwritten.
    

    Don't think this is throw an error. Just providing a nice warning message in large friendly letters?

  10. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -42,6 +44,13 @@ class Sql extends PluginBase implements MigrateIdMapInterface, ContainerFactoryP
    +   * The migration plugin manager to have access to the list of migrations.
    

    Is the phrase, 'to have access to the list of migrations.' needed? It reads funny to me.

  11. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +940,48 @@ public function valid() {
    +    // Look in all the derived migrations' mapping tables for their highest ID.
    

    At first I thought this foreach was just for derived migrations. It isn't. Can we change the comment to say that it gets the ID from all tables in the list?

  12. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6AuditIdsTest.php
    @@ -0,0 +1,197 @@
    +    $this->assertTrue(empty($conflicts));
    

    Why not use $this->assertEmpty?

quietone’s picture

FileSize
5.76 KB

Tested the patch on a clean install and the warning message was not displayed. Canceled that and then used Devel to general taxonomies, users, content and menus. The warning screen displayed but unexpectedly it lists 'files' twice. Maybe one for private files and one for public files? It was a d7 site I tested with. I expected the list to include users as well, I have made 50 users.

heddn’s picture

Status: Needs work » Needs review
FileSize
50.94 KB
10.98 KB

All feedback in #93 and #94 is addressed.

Status: Needs review » Needs work

The last submitted patch, 95: 2876085-95.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
50.93 KB
678 bytes
quietone’s picture

@heddn, thx. Yes, looks like all my points are addresses. Glad to see the audit_id configuration explained in Migration.php. Much better.

Haven't run a manual test, yet.

And, at what point can we get UX folks to look at the warning screen?

quietone’s picture

Status: Needs review » Needs work
FileSize
7.8 KB

Ran the test and the results look perfect! Both public and private files are shown and so are users. I also generated comments this time, and they are included in the warning notice.

Are translations being done here?

Setting to NW for the change record.

Warning screen snapshot

quietone’s picture

Using the same site I added translations for a node, comment, term and menu. The warning message now includes 'node translations' but not the comments, terms or menu.

quietone’s picture

Issue tags: +Usability
FileSize
20.3 KB

Discussed at the migrate meeting and I agree with heddn's suggestion in #91. That is, provide warnings about translations, documentation how to handle it, and make follow up issues. As much as I want to be inclusive waiting to got these 'tricky' issues solved will just take too long. And sites using translations are more likely to need custom migrations.

Also, tagging Usability for the warning screen.

And, from the call I realized that the testing I did above was with a D7 site. The previous warning is correct when migrating from a D7 site.

I repeated the test with s D6 source sites. The warning message is again correct.

heddn’s picture

Status: Needs work » Needs review

based on #101, I think this can go back to NR.

I think the outstanding things are:

  • architecture review
  • add change record
  • usability review
heddn’s picture

Issue summary: View changes
FileSize
66.4 KB

Updated IS and added a screenshot to it to aid in UX review.

heddn’s picture

And found an issue when looking at that screen. Fixed and posting new patch.

heddn’s picture

Issue summary: View changes
FileSize
50.85 KB
1.08 KB
64.63 KB
phenaproxima’s picture

Status: Needs review » Needs work

This is pretty nice and clear, but I have a lot of concerns about the current state of the patch, including architectural concerns.

  1. +++ b/core/modules/migrate/migrate.services.yml
    @@ -30,3 +30,5 @@ services:
    +  migrate.id_auditor:
    +    class: Drupal\migrate\MigrateIdAuditor
    

    This service has no dependencies or state, so it doesn't need to be a service. Consuming code can simply create a new MigrateIdAuditor().

  2. +++ b/core/modules/migrate/src/MigrateIdAuditor.php
    @@ -0,0 +1,59 @@
    +      if (!empty($definition['audit_ids'])) {
    

    This is a nit, but to reduce the amount of nesting (thus increasing readability), can we do this instead:

    if (empty($definition['audit_ids'])) continue;
    
  3. +++ b/core/modules/migrate/src/MigrateIdAuditor.php
    @@ -0,0 +1,59 @@
    +   * Build conflict label.
    

    This could use more detail -- perhaps a longer description, after the introductory line?

  4. +++ b/core/modules/migrate/src/MigrateIdAuditor.php
    @@ -0,0 +1,59 @@
    +   * @param \Drupal\migrate\Plugin\MigrationInterface $migration
    +   *
    +   * @return array
    

    These are both missing descriptions.

  5. +++ b/core/modules/migrate/src/MigrateIdAuditor.php
    @@ -0,0 +1,59 @@
    +    $base_id = $migration->getBaseId();
    

    I don't understand why we're keying by the base ID only. Wouldn't we want to collect more specific information, and key by plugin ID?

  6. +++ b/core/modules/migrate/src/MigrateIdAuditor.php
    @@ -0,0 +1,59 @@
    +      $conflict[$base_id] = $label->render();
    +      if (isset($label->getArguments()['@label'])) {
    +        $conflict[$base_id] = $label->getArguments()['@label'];
    +      }
    

    This is a bit messy, and I don't like the repeated calls to getArguments(). Can it look more like this:

    $arguments = $label->getArguments();
    
    if (isset($arguments['@label'])) {
      $conflict[$base_id] = $arguments['@label'];
    }
    else {
      $conflict[$base_id] = $label->render();
    }
    
  7. +++ b/core/modules/migrate/src/MigrateIdAuditorInterface.php
    @@ -0,0 +1,27 @@
    +   * the former are greater than the latter, there are ID conflicts.
    

    Nit: Should be "if the former is greater than..."

  8. +++ b/core/modules/migrate/src/MigrateIdAuditorInterface.php
    @@ -0,0 +1,27 @@
    +   * @return string[]
    +   *   The entity type IDs of migrated content that may have conflicting IDs.
    +   *   If no conflicts are found, an empty array will be returned.
    

    The description here seems to conflict with what I'm seeing in buildConflict(). At the very least, buildConflict() needs some inline comments to clarify.

  9. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationAuditInterface.php
    @@ -0,0 +1,44 @@
    +/**
    + * Provides an interface for destination audits.
    + *
    + * Destination plugins that are able to pre-audit the migration for conflicting
    + * IDs should implement this interface.
    + */
    

    Maybe add a @see comment or two referencing MigrateIdAuditorInterface?

  10. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationAuditInterface.php
    @@ -0,0 +1,44 @@
    +interface MigrateDestinationAuditInterface {
    

    This is a long class name. Seeing as how this is already in Migrate's namespace, would it be kosher to rename this to DestinationAuditInterface?

  11. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationAuditInterface.php
    @@ -0,0 +1,44 @@
    +   *   The highest ID found. If no IDs are found, or if the concept of a highest
    +   *   ID is not meaningful, zero should be returned.
    

    I'm not sure zero should be an acceptable return value. I would think that, if the concept of a highest ID is not meaningful, the destination should not be implementing this interface.

  12. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationAuditInterface.php
    @@ -0,0 +1,44 @@
    +  /**
    +   * Gets whether this destination is for translations.
    +   *
    +   * @return bool
    +   *   Whether this destination is for translations.
    +   */
    +  public function isTranslationDestination();
    

    How is this related to ID auditing? I think this might need more explanation in the doc comment.

  13. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapAuditInterface.php
    @@ -0,0 +1,23 @@
    +interface MigrateIdMapAuditInterface {
    

    Okay, so...as far as I can tell, we are now going to be providing two separate interfaces to do the same exact thing, which is "return the highest value". This surely could be a single interface implemented by both ID maps and destinations: MaximumInterface or something. Not sure how to name it. Hell, maybe even just \Countable would suffice. Then we wouldn't have to implement stuff like getAuditedIdFieldName() on destinations that support auditing.

    Point is, I feel like we're adding a lot of moving parts to Migrate indiscriminately. The API is already very expansive and difficult to grok, so I feel like we should be sparing about adding even more moving parts when existing things (like \Countable) could work just fine.

  14. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -16,6 +16,13 @@
    + *   - audit_ids: If set to TRUE, this migrations's IDs will be audited. This
    + *     means that, for the entity type this migration creates, if the highest
    + *     destination ID is greater than the highest source ID, a warning will
    + *     be displayed that entities might be overwritten.
    

    Let's add a @see referencing MigrateIdAuditorInterface.

  15. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapEnsureTablesTest.php
    @@ -232,7 +232,8 @@ protected function runEnsureTablesTest($schema) {
    +    $migration_plugin_manager = $this->getMock('Drupal\migrate\Plugin\MigrationPluginManagerInterface');
    +    $map = new TestSqlIdMap($database, [], 'sql', [], $migration, $event_dispatcher, $migration_plugin_manager);
    

    It'd be preferable to use Prophecy here (also ::class syntax, rather than writing out the entire class name).

  16. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
    @@ -111,8 +111,9 @@ protected function getIdMap() {
    +    $migration_plugin_manager = $this->getMock('Drupal\migrate\Plugin\MigrationPluginManagerInterface');
    

    Ditto.

  17. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7AuditIdsTest.php
    @@ -0,0 +1,197 @@
    +class MigrateDrupal7AuditIdsTest extends MigrateDrupal7TestBase {
    

    There's a lot of commonality between this test and the D6 one. Can we combine some of the shared functionality in a trait?

  18. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -419,6 +437,121 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +  public function buildIdConflictForm(array &$form, FormStateInterface $form_state) {
    

    Is $form supposed to be passed by reference?

  19. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -419,6 +437,121 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +      if ($migration->getDestinationPlugin() instanceof MigrateDestinationAuditInterface) {
    +        if ($migration->getDestinationPlugin()->isTranslationDestination()) {
    

    Let's not repeatedly call getDestinationPlugin().

  20. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -419,6 +437,121 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +  protected function conflictsForm(array &$form, FormStateInterface $form_state, $conflicts) {
    

    Not sure $form should be a reference.

  21. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -419,6 +437,121 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +  protected function i18nWarningForm(array &$form, FormStateInterface $form_state, $i18n) {
    

    Again, not sure $form should be a reference here.

  22. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -610,4 +743,31 @@ public function getConfirmText() {
    +   * @TODO: This is a duplicate of MigrateIdAuditor::buildConflict() and should
    +   * eventually be removed in https://www.drupal.org/node/2905759
    

    I don't get why we'd clone this. How about just making MigrateIdAuditor::buildConflict() a public static method, so it can be reused? In fact, we could add that to the interface too.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
44.81 KB

This patch refactors #105 and addresses a lot of my expressed concerns. I condensed a bunch of the stuff we had and introduced only one interface for destinations and ID maps to share. Amazingly...this passed the tests on the first try. O_o

Not bothering with an interdiff this time, because the changes are numerous enough make it impossible to read. :)

Status: Needs review » Needs work

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

phenaproxima’s picture

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

#107 was rolled against 8.5.x.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
44.56 KB

Unable to replicate test fails locally so corrected minor coding standards errors and resubmitting to the testbot. Will look again if the errors occur.

Status: Needs review » Needs work

The last submitted patch, 110: 2876085-110.patch, failed testing. View results

maxocub’s picture

+++ b/core/modules/user/migration_templates/d7_user.yml
@@ -1,5 +1,6 @@
+class: '\Drupal\migrate\Plugin\IdAuditingMigration'
 migration_tags:
   - Drupal 7
 class: Drupal\user\Plugin\migrate\User

The tests are failing because there's two class keys in d7_user.

Before, we were using a tag 'audit_ids', but if we are now using class, I don't know how to fix this. Any idea @phenaproxima?

heddn’s picture

maxocub’s picture

Nice, I haven't realized that IdAuditingMigration was extending Migration, so I guess that will fix it.

Status: Needs review » Needs work

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

heddn’s picture

Here's a quick review.

  1. +++ b/core/modules/migrate/src/Plugin/IdAuditingMigration.php
    @@ -0,0 +1,30 @@
    +    if ($destination instanceof MaximumValueInterface && $id_map instanceof MaximumValueInterface && $destination->max() > $id_map->max()) {
    

    Do we really need this? If you are using the IdAuditingMigration, can we just assume a certain set of implementation details? I almost want to throw an exception/warning if these don't implement, rather than just silently skip.

  2. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -16,6 +16,13 @@
    + *   - audit_ids: If set to TRUE, this migrations's IDs will be audited. This
    

    This isn't needed/used any more.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -111,12 +112,9 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
    +  public function isTranslationDestination() {
    
    +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -430,6 +434,125 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +        if ($migration->getDestinationPlugin()->isTranslationDestination()) {
    

    We cannot assume this will be available from the destination, unless we check if it is EntityContentBase, no?

heddn’s picture

Status: Needs work » Needs review
FileSize
47.37 KB
3.44 KB

Hopefully this picks up the failures and my feedback from #113. I also added an interface for the auditing and moved over the comments to its class doxygen from the base migration plugin class.

Status: Needs review » Needs work

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

heddn’s picture

Status: Needs work » Needs review
FileSize
47.36 KB
517 bytes

Status: Needs review » Needs work

The last submitted patch, 119: 2876085-119.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
46.33 KB
3.08 KB

Good call adding the InvalidPluginDefinitionException, it caught that we were trying to audit field migrations, when that simply won't work.

Status: Needs review » Needs work

The last submitted patch, 121: 2876085-121.patch, failed testing. View results

xjm’s picture

Priority: Major » Critical

Per discussion with @catch, we're promoting the "Migrate critical" issues to critical given the importance of a stable migration path at this point in the release cycle. (The framework and release managers will confirm this in a later triage for individual issues.)

phenaproxima’s picture

I did some detective work and determined, as you guys already did (I should read more) that the patch in #121 is failing tests is because UserMigration extends FieldMigration, which doesn't extend IdAuditingMigration.

I think the solution is to make the contents of IdAuditingMigration a trait, which can fulfill the implementation of an interface (AuditableMigrationInterface is one possibility for what to call that). Then we can use it on anything, without worrying about breaking the inheritance chain. That should fix it. Then we can get this thing in.

maxocub’s picture

Adding a tag so we can see this issue in the kanban.

maxocub’s picture

Issue tags: +Vienna2017

Oups, forgot the tag.

maxocub’s picture

Assigned: Unassigned » maxocub

Assigning for fixing the test and adding the trait.

phenaproxima’s picture

Assigned: maxocub » phenaproxima

I'm going to reassign this to myself because honestly, my thinking about this patch is kind of scattered and I want to settle down on a preferred architecture rather than send someone else through a labyrinthine maze of my personal thought process.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
46.5 KB

Let's try this on for size. It passed all Migrate tests locally...

No interdiff because it's significantly different from #121.

phenaproxima’s picture

Added doc comments.

heddn’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/migration_templates/d7_file.yml
    @@ -1,7 +1,8 @@
    +label: Public Files
    
    +++ b/core/modules/file/migration_templates/d7_file_private.yml
    @@ -1,5 +1,6 @@
    +label: Private Files
    

    Let's move this to another issue. That would let a fairly trivial change get fixed pretty easily.

  2. +++ b/core/modules/migrate/src/Audit/AuditException.php
    @@ -0,0 +1,24 @@
    +    $message = 'Cannot audit migration ' . $migration->id() . ': ' . $message;
    
    +++ b/core/modules/migrate/src/Audit/IdAuditor.php
    @@ -0,0 +1,68 @@
    +      throw new AuditException($migration, 'Destination plugin does not implement ' . MaximumValueInterface::class);
    ...
    +      throw new AuditException($migration, 'ID map plugin does not implement ' . MaximumValueInterface::class);
    

    Nit: sprintf would make this easier to read.

  3. +++ b/core/modules/migrate/src/Audit/IdAuditor.php
    @@ -0,0 +1,68 @@
    +    $base_id = $migration->getBaseId();
    ...
    +      $conflict[$base_id] = $label;
    ...
    +      $conflict[$base_id] = isset($arguments['@label']) ? $arguments['@label'] : $label->render();
    

    Won't this cause d6_node:page and d6_node:blog to overwrite each other? With a key of d6:node.

  4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -111,12 +112,9 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
    +  public function isTranslationDestination() {
    

    This isn't on the interface, so we cannot trust it will exist. Ah, I see. We are checking later on the form if this is an ECB. That's fine.

  5. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -152,11 +161,16 @@ class Sql extends PluginBase implements MigrateIdMapInterface, ContainerFactoryP
         parent::__construct($configuration, $plugin_id, $plugin_definition);
    ...
    +    $this->migrationPluginManager = $migration_plugin_manager;
    

    Does this cause a BC break? It could break a lot of contrib tests as we'll have to add more to the mock object. We'll break BC if we need to, but I think this is the first real breaking change.

  6. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +940,59 @@ public function valid() {
    +      throw new \LogicException('TODO');
    

    This means either needs work to add the issue to fix this or we need to add the logic now.

quietone’s picture

No interdiff because it's significantly different from #121.

To me this sounds like the whole approach is different. Can you say a few words about that?

phenaproxima’s picture

The whole approach is not enormously different; there are just enough changes between the two patches that there wasn't going to be much benefit to an interdiff. I can post one anyway if you want.

heddn’s picture

I've added #2916642: Private and Public file migrations use the same name. We can still keep those changes in this patch, but let's hope that the follow-up will get committed first.

quietone’s picture

@phenaproxima, no need for an interdiff. Thanks for the offer.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Migrate BC break
FileSize
43.8 KB
8.34 KB

Addressed all of @heddn's points in #131. Except for:

Won't this cause d6_node:page and d6_node:blog to overwrite each other? With a key of d6:node.

Yes, it will, but since both migrations have the same destination, and that destination's max() method is not filtering by node type, conflicts detected for any given derivative of d6_node will be detected for any other derivative. All migrations that are opting into auditing follow a similar pattern. So it makes sense for the base ID to be used as the key for the conflicts.

I'm un-tagging this as a BC break, because...it doesn't break BC.

quietone’s picture

I read the code (skimmed the changed to all the yml) but didn't apply the patch and test this time. Most of what gives me pause is the language in the displayed messages.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +929,63 @@ public function valid() {
    +    if ($base_id = substr($migration_id, 0, strpos($migration_id, ':'))) {
    

    Interesting. Can this use DERIVATIVE_SEPARATOR?

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -434,6 +439,125 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +    $i18n = $conflicts = [];
    

    Just a bit confused by the names. To me, i18n refers to the i18n module. But, here i18n is being used to identify 'translation' destinations. If one of these is for a destination then what is conflicts? I guess it is content_conflicts and translated_content_conflicts.

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -434,6 +439,125 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +      '#markup' => '<h3>' . $this->t('Entities may be overwritten') . '</h3>',
    

    Can we expect all users of the UI to know what an entity is?

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -434,6 +439,125 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +    $form['warning']['#markup'] .= '<p>' . $this->t('Upgrades work on brand new sites, or sites with previously upgraded data. However, it looks like you have content in your site that is unknown to the migrate system; perhaps manually added. These new entities <strong>may be overwritten</strong> if you run this upgrade. For more information, see the online handbook entry for <a target="_blank" href=":id-conflicts-handbook">handling migration conflicts</a>.', [':id-conflicts-handbook' => 'https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8#id_conflicts']) . '</p>';
    

    This should use the same language as on /upgrade.
    s/brand new sites/clean and empty new install of Drupal 8.

    And 'sites with previously upgraded data' conflicts with having a clean and empty new install.

  5. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -434,6 +439,125 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +      '#title' => $this->t('The conflicting entities are of the following types:'),
    

    Ditto about entities.

  6. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -434,6 +439,125 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +      '#title' => $this->t('The translatable entities are of the following types:'),
    

    s/translatable entities/translated content/

  7. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -152,8 +152,10 @@ public function testMigrateUpgrade() {
    +    $this->assertSession()->pageTextContains('Entities may be overwritten');
    

    Again with entities

  8. +++ b/core/modules/user/migration_templates/d7_user.yml
    @@ -1,8 +1,9 @@
    +class: '\Drupal\user\Plugin\migrate\User'
    

    quotes not needed.

heddn’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
phenaproxima’s picture

Status: Needs work » Needs review

Those name changes were removed in #136, so I don't think we need to reroll this...

Status: Needs review » Needs work

The last submitted patch, 136: 2876085-136.patch, failed testing. View results

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
43.82 KB

It needed a re-roll because of the change in context lines. Here it is.

Jo Fitzgerald’s picture

Assigned: phenaproxima » Jo Fitzgerald
Status: Needs review » Needs work

Addressing @quietone's review in #137.

Jo Fitzgerald’s picture

Assigned: Jo Fitzgerald » Unassigned
Status: Needs work » Needs review
FileSize
8.71 KB
43.89 KB

Response to #137:

  1. Use DERIVATIVE_SEPARATOR.
  2. Replaced $i18n with $translated_content_conflicts and $conflicts with $content_conflicts.
  3. Replaced "Entities" with "Content".
  4. Use the same language as on /upgrade. Removed 'sites with previously upgraded data'.
  5. Replaced "Entities" with "Content".
  6. Changed.
  7. Replaced "Entities" with "Content".
  8. Removed quotes.
maxocub’s picture

Status: Needs review » Needs work

I very much like the new approach. Every thing seems clear. I just have a few nits and a question:

  1. +++ b/core/modules/migrate/src/Audit/AuditorInterface.php
    @@ -0,0 +1,44 @@
    +   * @throws \Drupal\migrate\Audit\AuditException if the audit fails.
    

    Comment should be on next line.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -169,7 +172,8 @@ public static function create(ContainerInterface $container, array $configuratio
    +      $container->get('plugin.manager.migration')
    

    I don't think that's needed anymore.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +929,63 @@ public function valid() {
    +      // @TODO Inject the plugin manager as a dependency.
    +      /** @var \Drupal\migrate\Plugin\MigrationPluginManagerInterface $migration_manager */
    +      $migration_manager = \Drupal::service('plugin.manager.migration');
    

    So I understand that this is for not breaking BC, but just out of curiosity, when will this @TODO be addressed? Drupal 9?

phenaproxima’s picture

So I understand that this is for not breaking BC, but just out of curiosity, when will this @TODO be addressed? Drupal 9?

I think it'll have to be.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
43.8 KB
1.07 KB

Addressed points 1 and 2 from #144.

Jo Fitzgerald’s picture

Status: Needs review » Reviewed & tested by the community

#146 addresses the first 2 points from #144, #145 addresses the 3 point. Therefore this can be moved to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 146: 2876085-146.patch, failed testing. View results

phenaproxima’s picture

Issue tags: +Needs reroll

Failed to apply? Methinks a reroll is in order.

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
42.94 KB

Here it is!

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC. I think this was caused by #2905227: Migrate UI: Improve 'Review Upgrade' page UX

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

This is progressing well - nice work folks

  1. +++ b/core/modules/migrate/src/Audit/IdAuditor.php
    @@ -0,0 +1,82 @@
    +      $arguments = $label->getArguments();
    +      $conflict[$base_id] = isset($arguments['@label']) ? $arguments['@label'] : $label->render();
    

    This is a bit nasty. Are we sure we want to embed this kind of deep knowledge of translatable markup behaviour here? What is the cost of just using $conflict[$base_id] = (string $label;

  2. +++ b/core/modules/migrate/src/Audit/MaximumValueInterface.php
    @@ -0,0 +1,26 @@
    +  public function max();
    

    I think the name here should be a bit more verbose, e.g getMaximumId(), max doesn't really convey what is going on that clearly

  3. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -16,6 +16,13 @@
    + *     that, for the entity type this migration creates, if the highest
    

    nit: the second sentence here is disjointed.

    suggest: 'If the highest destination is greater than the highest source ID, a warning will be displayed that entities might be overwritten'

  4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -111,12 +112,9 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
    -  protected function isTranslationDestination() {
    +  public function isTranslationDestination() {
    

    intended change?

  5. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +928,63 @@ public function valid() {
    +    $destination_ids = array_filter(
    

    What if there is more than one ID field? Do we care? Should we throw an Exception because all bets are off?

  6. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +928,63 @@ public function valid() {
    +        return $id['type'] == 'integer';
    

    nit, ===

  7. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +928,63 @@ public function valid() {
    +      // @TODO Inject the plugin manager as a dependency.
    +      /** @var \Drupal\migrate\Plugin\MigrationPluginManagerInterface $migration_manager */
    +      $migration_manager = \Drupal::service('plugin.manager.migration');
    

    let's do that here

  8. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +928,63 @@ public function valid() {
    +        if ($migration['id'] == $base_id) {
    

    nit ===, these are strings

  9. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +928,63 @@ public function valid() {
    +    // Return the highest of the highest IDs.
    

    the highest of the highest?

  10. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
    @@ -7,7 +7,6 @@
    -use Drupal\migrate\Plugin\Migration;
    
    @@ -47,6 +46,9 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    

    out of scope/unintended?

  11. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6AuditIdsTest.php
    @@ -0,0 +1,195 @@
    +    $this->createContent();
    

    nice

  12. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6AuditIdsTest.php
    @@ -0,0 +1,195 @@
    +  protected function createContent() {
    

    missing a docblock?

  13. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7AuditIdsTest.php
    @@ -0,0 +1,194 @@
    +  protected function createContent() {
    

    duplicated? could go in a trait and share between both tests?

  14. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7AuditIdsTest.php
    --- a/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    

    sidenote: this form is getting rather large now. follow-up to split it into separate helper classes for each step for maintenance sake?

  15. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -434,6 +439,125 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +    $auditor = new IdAuditor();
    

    If we have an interface for auditors, and the description above effectively says 'could audit in any way they see fit', how does one use a custom auditor? Should this be done in an extensible way?

  16. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -434,6 +439,125 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +      if ($destination_plugin instanceof EntityContentBase && $destination_plugin->isTranslationDestination()) {
    

    ah nvm comment above about protected->public change

  17. +++ b/core/modules/user/migration_templates/d7_user.yml
    @@ -1,8 +1,9 @@
    -class: Drupal\user\Plugin\migrate\User
    +class: \Drupal\user\Plugin\migrate\User
    

    intended?

  18. +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
    @@ -127,4 +127,18 @@ protected function processStubRow(Row $row) {
    +    if ($highest_id == 1) {
    

    nit ===, the parent casts the return to an integer so we should be type-safe here

Also, looks like we still need a change record?

heddn’s picture

Working to address feedback in #152.

heddn’s picture

All but #5 from #152 is now addressed. I agree we need to handle multiple destination ids. Paragraphs requires it. I just don't have time to work on it right now. I'm figuring we should do something similar to EntityContentBase::getIds().
For #14, I've opened #2918761: Break up MigrateUpgradeForm into smaller forms

Setting to NR for testbot. We, still need to add a CR and address #5.

Status: Needs review » Needs work

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

maxocub’s picture

Status: Needs work » Needs review
FileSize
47.88 KB
4.51 KB

This should fix the failing tests.

These fails show that this BC break might cause a lot of test failures in contrib, as mentioned in #131.5.

maxocub’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -84,6 +86,13 @@ class Sql extends PluginBase implements MigrateIdMapInterface, ContainerFactoryP
+  protected $migration_plugin_manager;

@@ -152,12 +161,17 @@ class Sql extends PluginBase implements MigrateIdMapInterface, ContainerFactoryP
+    $this->migration_plugin_manager = $migration_plugin_manager;

@@ -925,4 +940,59 @@ public function valid() {
+      $migrations = $this->migration_plugin_manager->getDefinitions();
...
+          $stub = $this->migration_plugin_manager->createInstance($migration_id);

This property should be in lowerCamelCase.

maxocub’s picture

I don't know if we can do that, but one way we could avoid the BC break is by changing the visibility of the getMigrationPluginManager() method on the Migration Plugin:

  /**
   * Gets the migration plugin manager.
   *
   * @return \Drupal\migrate\Plugin\MigratePluginManager
   *   The plugin manager.
   */
  protected function getMigrationPluginManager() {
    return $this->migrationPluginManager;
  }

If it was public, the Sql class could have access to the manager without changing it's constructor.

Is that something we can do or does it have a very good reason for being protected?

quietone’s picture

#152.14 - Glad to see this. I've been wanting to do that for some time. Even tried when I was just learning but found I prefer migraty stuff to forms.

maxocub’s picture

Status: Needs review » Needs work

Back to NW for #152.5 and the change record.

heddn’s picture

re #158, one good reason to not do that is getMigrationPluginManager is not on the MigrationInterface. So you cannot be always sure that it will be available. We're stuck with either not DI and saving BC or doing DI and breaking BC.

heddn’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
47.58 KB

Let's see how this fares with tests. I've picked up #152.5

maxocub’s picture

Status: Needs review » Needs work

We should do two change records, one for the BC break (Sql class) and one for the new audit step.

heddn’s picture

Issue tags: +Migrate BC break

Tagging for BC break. The alternatives are to add an TODO and open a follow-up issue for 9.x to fix this more better.

larowlan’s picture

I think if we're breaking BC just for DI, we should just use the singleton behind a protected accessor with a todo to resolve in 9.0

Plugin constructors are considered internal, so we don't have to retain BC, but providing a change-record is a considerate thing to do.

We had this discussion recently, where I pointed out that those sub-classing plugins should use setter methods from the factory.


public static function create($stuff) {
  // Call the parent.
  $instance = parent::static($stuff);
  $instance->setSomethingElse($something);
  return $instance;
}

This means you can extend a plugin, and be insulated against changes in the parent constructor.

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Migrate BC break
FileSize
43.58 KB
6.91 KB

My understanding of #165 is that we can either break BC if we provide a nice change record, or not break BC and add a todo for Drupal 9.

So let's not break BC since Migrate is in beta. Here's the follow up for Drupal 9: #2919158: Add the MigrationPluginManager to Drupal\migrate\Plugin\migrate\id_map\Sql.

(We can still go back and forth with this BC / no BC thing if everyone is not on board.)

heddn’s picture

I like it. But I cannot RTBC.

quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
34.31 KB

I started reviewing last night and now have time to finish and comment.

From the review in #152, everything has been done, except #4. That was a asking a question which has not been explicitly answered. The question was if the change from protected to public on isTranslationDestination is intended. The answer is yes. That method is part of the destination plugin EntityContentBase and until now it was only needed by destinations. The auditor needs to know about translations to function properly.

That leaves responding to #165, which maxocub did and heddn likes. I don't quite get that so will refrain from comment.

I tested the patch, with D7 and 1 article. A screen shot is attached. I am still concerned about the text. I think it is a bit long and still mentions entities. And 'Conflicting content' is a strange way to say, 'these may be destroyed'. What is the way to get UI input to this? Or do we do that in follow up? Sorry, marking as NW for this.

Updated the screenshot in the IS to use this one.

quietone’s picture

+++ b/core/modules/migrate/src/Audit/MaximumValueInterface.php
@@ -0,0 +1,26 @@
+  public function getHighestId();

Oh, and why 'highest' and not 'maximum'?

maxocub’s picture

Re #169: I'm not sure which one is better, 'getHighestId()', 'getMaximumId()' or 'getMaximumValue()' (since it's the MaximumValueInterface)?

Re #168: There's actually a problem in your screenshot, the fact that the label is displaying the node type (Article).

It's ok in your case because you added an article, but if you added a page, it would also display 'Node (Article)'. The problem is that the auditor, have no idea what the bundle is and it should just display the entity type. That is what we were doing before with the clumsy translatable markup and arguments:

+++ b/core/modules/migrate/src/Audit/IdAuditor.php
@@ -0,0 +1,82 @@
+  public static function conflict(MigrationInterface $migration) {
...
+    $label = $migration->label();
+    if (is_string($label)) {
+      $conflict[$base_id] = $label;
+    }
+    elseif ($label instanceof TranslatableMarkup) {
+      $arguments = $label->getArguments();
+      $conflict[$base_id] = isset($arguments['@label']) ? $arguments['@label'] : $label->render();
+    }
...
+  }

That way it was only displaying 'Node' or 'Node revision' without the (maybe) wrong and misleading bundle.

This was removed after @larowlan's comment #152:

This is a bit nasty. Are we sure we want to embed this kind of deep knowledge of translatable markup behaviour here? What is the cost of just using $conflict[$base_id] = (string) $label;

I don't know how it could be done otherwise, any ideas?

quietone’s picture

Re #169: I'm not sure which one is better, 'getHighestId()', 'getMaximumId()' or 'getMaximumValue()' (since it's the MaximumValueInterface)?

Given those choices, I prefer using a similar name as the interface, either getMaximumId or getMaximumValue. This is really looking as ids so getMaximumId(). But, if everyone else is happy with getHighestId(), I can live with the status quo.

Re #168. Good catch. I just added a single page and tested again. Sure enough, the message states that the conflicting content is Article node types no mention of Page. Clearly this needs to be changed. We can either have a generic message, "One or more of the Content Types on this site" (not suggesting that text, just making the point) or return to the more specific one.

As a user, I would prefer to know exactly content type I might lose. If there is agreement on that the user should know, then restore the code, add a explanatory comment that this isn't ideal. And a follow to fix it if and when a better way is found.

larowlan’s picture

Yeah agree - my comment was more of an observation, even if we had a helper class with the same logic in it would be better

heddn’s picture

In the migrate maintainers meeting in IRC we discussed this issue. In attendance were @rakeshjames, @phenaproxima, @quietone, @maxocub, @heddn.

We decided to use Highest for the interface and for labels the following:

We like "node" (or rather "content", the human-readable plural label of the node entity type) without bundle -- but we have to find a way to do that without assuming the destination is an entity destination. To do that, get the destination plugin's label. $destination->getPluginDefinition()['label']. And just have EntityContentBase override that, in the constructor, with the human-readable name of the entity type it's going to save. That way we are compatible with all destinations, and the auditor is not making assumptions.

maxocub’s picture

Status: Needs work » Needs review
FileSize
42.95 KB

Status: Needs review » Needs work

The last submitted patch, 174: 2876085-174.patch, failed testing. View results

maxocub’s picture

Status: Needs work » Needs review
FileSize
42.9 KB
1.42 KB

Oups, 2nd try.

maxocub’s picture

Assigned: Unassigned » maxocub
Status: Needs review » Needs work

We discussed this issue in yesterday's maintainers meeting on IRC with @heddn, @phenaproxima, @quietone, @masipila and myself.

We thought that the auditor should not return only migration labels, but that it should return an object (AuditResult?) which references the migration, and also an array of strings (information), plus boolean flags indicating if the audit passed or failed, with getter methods to access those properties (getMigration(), getReasons(), getStatus()?).

phenaproxima’s picture

I propose the following things about AuditResult:

  • AuditResult will implement MarkupInterface and Countable, so that it can be used in Twig templates, and be XSS filtered by the rendering system. This allows the reasons to include markup.
  • In addition to a migration, AuditResult will be constructed with a status that must be boolean (not an int, string, or anything else). Once that status is set, it cannot be changed.
  • AuditResults can have several "reasons" attached to them, which are strings or objects with __toString() methods. When cast to strings themselves (i.e., MarkupInterface), AuditResults return the collected reasons as a string.
  • For better DX, AuditResult will include static pass() and fail() factory methods, much like AccessResult does.

Is there any reason not to do these things?

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
45.47 KB
11.03 KB

Here's a first attempt at implementing all the stuff that has been discussed amongst the maintainers since @larowlan's review.

Status: Needs review » Needs work

The last submitted patch, 179: 2876085-179.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
50.87 KB
8.67 KB

I should try running the tests first. Hopefully this fixes 'em.

masipila’s picture

Hi,

Nice to see how this is evolving! A couple of cosmetic coding standard nits.

1. Class descriptions should start with a third person verb.

+++ b/core/modules/migrate/src/Audit/AuditException.php
@@ -0,0 +1,27 @@
+<?php
+
+namespace Drupal\migrate\Audit;
+
+use Drupal\migrate\Plugin\MigrationInterface;
+
+/**
+ * Exception thrown when a migration audit fails.
+ */
+class AuditException extends \RuntimeException {

2. Why don't we just call this getStatus()?

+++ b/core/modules/migrate/src/Audit/AuditResult.php
+  /**
+   * Returns the boolean result of the audit.
+   *
+   * @return bool
+   *   The result of the audit. TRUE if the migration passed the audit, FALSE
+   *   otherwise.
+   */
+  public function passed() {
+    return $this->status;
+  }

3. Extra line break here before the bullet list.

+++ b/core/modules/migrate/src/Plugin/Migration.php
+ * Configuration options:
+ *
+ *   - audit: If set to TRUE, the migration's IDs will be audited. This means
+ *     that if the highest destination ID is greater than the highest source ID,
+ *     a warning will be displayed that entities might be overwritten.
  */

4. Description should start with a third person verb i.e. 'Returns the migration plugin manager'.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
+  /**
+   * Get the migration plugin manager.
+   *
+   * @todo Inject as a dependency in https://www.drupal.org/node/2919158.
+   *
+   * @return \Drupal\migrate\Plugin\MigrationPluginManagerInterface
+   *   The migration plugin manager.
+   */
+  protected function getMigrationPluginManager() {
+    return \Drupal::service('plugin.manager.migration');
+  }

Markus

masipila’s picture

Status: Needs review » Needs work
phenaproxima’s picture

Status: Needs work » Needs review
FileSize
50.89 KB
1.78 KB

All fixed, except for this one:

Why don't we just call this getStatus()?

In the context of a value object encapsulating the result of a testing process, I think passed() more clearly expresses the intent of the method in calling code. I'll change it if enough people want me to, but for now I like it the way it is :)

masipila’s picture

I manually tested #184 with both Migrate Drupal UI (i.e. web browser user interface) and with Drush.

  • When using Migrate Drupal UI (i.e. /upgrade), I was able to see the warnings as expected when I had manually created content on D8 site before upgrading.
  • Was there supposed to be something shown when migrating using Drush? Or is this something that Migrate Upgrade or Migrate Tools could implement as a follow-up?

I have read through the patch and commented in #182 the nits that I was able to spot and those have now been addressed. I think @heddn and @maxocub have been contributing to the patch so @quietone, could you please give this another pair of eyeballs and RTBC this when you have had time to review this?

Cheers,
Markus

phenaproxima’s picture

Was there supposed to be something shown when migrating using Drush? Or is this something that Migrate Upgrade or Migrate Tools could implement as a follow-up?

They'll have to address it in follow-ups, since it's the responsibility of the calling code to instantiate the auditor(s) and run them.

heddn’s picture

re #185: I was going to suggest we open a follow-up for migrate tools, then I found #2840154: Implement migrate-audit command for D8.

yoroy’s picture

Just skimming the t() strings here I found "I acknowledge I may lose data, continue anyway.", so linking #2773205: Come up with a design for highly destructive operations in confirm forms here :)

masipila’s picture

Re: #188. I created a follow-up for improving this warning UI, see #2923052: Improve the UI of upgrade page when ID conflicts are detected.

I read through the patch again today on my way home from work and I couldn't find a nit (I love all the 'wonkiness' and 'craziness' in the tests). However, I'll let the other maintainers to do a second review for this.

Cheers,
Markus

maxocub’s picture

Assigned: maxocub » Unassigned
Status: Needs review » Needs work
FileSize
21.48 KB

I really like the new AuditResult. Just a few nits:

  1. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -169,7 +172,8 @@ public static function create(ContainerInterface $container, array $configuratio
    +      $container->get('plugin.manager.migration')
    

    I don't think this is required anymore.

  2. +++ b/core/modules/migrate/tests/src/Unit/Plugin/migrate/destination/EntityContentBaseTest.php
    @@ -11,6 +11,7 @@
     use Drupal\Core\Entity\ContentEntityType;
    

    Unused use statement.

  3. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateDrupalTestBase.php
    @@ -3,6 +3,8 @@
    +use Drupal\migrate\Audit\AuditResult;
    +use Drupal\migrate\Audit\IdAuditor;
    

    Unused use statements

I also tested this patch with a D7 site with all the supported entities (aggregator feed, aggregator feed items, custom blocks, comments, custom menu links, files, nodes, node revisions, taxonomy terms, users). Here's what it looks like:

I think there's two 'file entities' because of the public and private files. I only uploaded one public file on the D8 site.
I think there's four 'content items' because I have two content types, Page & Article, and that node revisions are also displayed as 'content items'. Also, I only created 1 page on the D8 site.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
50.65 KB
7.26 KB

Thanks for the review and thorough test, @maxocub! This oughta fix all of that :)

maxocub’s picture

Status: Needs review » Needs work
FileSize
18.91 KB

I did a quick test of the new patch:

This is better.
The file entities now appears only once
The content items still appears twice.
I guess it's the 'node' and 'node revision' which use the same label...
Also, I was not able to display the warning about i18n.
I did create translations on both the D7 and the D8 site, so there should be a warning.

heddn’s picture

This hopefully disambiguates the revisions from the original content. Still need test coverage for it and figure out why i18n translations didn't appear.

maxocub’s picture

Re #193: Good idea for the revisions disambiguation, but let's also make it translatable.

About the i18n conflicts form not showing, it was because the translations are not yet audited so they are not yet failing, and thus they were not added to the conflicts array. This should fix this.

screenshot

Status: Needs review » Needs work

The last submitted patch, 194: 2876085-194.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
742 bytes
51.63 KB

Correct the test failures.

maxocub’s picture

Added tests for the conflicts on the migrate upgrade form.

xjm’s picture

maxocub’s picture

Let's not forget that we still need a change record. I can write it tomorrow if nobody do it before.

maxocub’s picture

Issue summary: View changes
Issue tags: -Needs change record
FileSize
55.22 KB
3.78 KB
  1. I changed MaximumValueInterface to HighestIdInterface as was decided in #173
  2. I added two change records, one for site builders about the new warning when upgrading from the UI and one for developers about the new classes for auditing migrations.
  3. I updated the issue summary.
neclimdul’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Audit/HighestIdInterface.php
    @@ -0,0 +1,26 @@
    + * exists, regardless of whether it was created by a migration.
    

    Is this how its different from watermarks? The method is confusingly close to me.

  2. +++ b/core/modules/migrate/src/Audit/HighestIdInterface.php
    @@ -0,0 +1,26 @@
    +   * Returns the highest ID tracked by the implementing plugin.
    

    s/implemented plugin/destination/ ?

  3. +++ b/core/modules/migrate/src/Audit/IdAuditor.php
    @@ -0,0 +1,60 @@
    +      $conflicts[$migration_id] = $this->audit($migration);
    +    }
    +    ksort($conflicts);
    +    return $conflicts;
    

    I'm not following the conflicts result here. Its just results right?

  4. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -16,6 +16,11 @@
    + *
    + * Configuration options:
    + * - audit: If set to TRUE, the migration's IDs will be audited. This means
    + *   that if the highest destination ID is greater than the highest source ID,
    + *   a warning will be displayed that entities might be overwritten.
    

    This should be an attribute shouldn't it? Seems we should go ahead and give it a sane default for contrib migrations.

  5. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
    @@ -94,6 +94,10 @@
    +    $plugin_definition += [
    +      'label' => $storage->getEntityType()->getPluralLabel(),
    +    ];
    +
    

    why?

  6. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -111,12 +112,9 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
    -   * Get whether this destination is for translations.
    -   *
    -   * @return bool
    -   *   Whether this destination is for translations.
    +   * {@inheritdoc}
        */
    -  protected function isTranslationDestination() {
    +  public function isTranslationDestination() {
    

    as was brought up in #106.12, not following this change. Don't see a follow up comment.

  7. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +928,69 @@ public function valid() {
    +  /**
    +   * Returns the migration plugin manager.
    +   *
    +   * @todo Inject as a dependency in https://www.drupal.org/node/2919158.
    +   *
    +   * @return \Drupal\migrate\Plugin\MigrationPluginManagerInterface
    +   *   The migration plugin manager.
    +   */
    +  protected function getMigrationPluginManager() {
    +    return \Drupal::service('plugin.manager.migration');
    +  }
    +
    

    this doesn't seem needed. isn't that forward compatibility exactly why the create method exists?

  8. +++ b/core/modules/migrate_drupal/tests/src/Kernel/CreateContentTrait.php
    @@ -0,0 +1,92 @@
    +trait CreateContentTrait {
    

    Handy for these tests but doesn't it seem a bit too generically named?

  9. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6AuditIdsTest.php
    @@ -0,0 +1,136 @@
    +    // Install required schemas.
    +    $this->installEntitySchema('aggregator_item');
    +    $this->installEntitySchema('aggregator_feed');
    +    $this->installEntitySchema('block_content');
    +    $this->installEntitySchema('comment');
    +    $this->installEntitySchema('file');
    +    $this->installEntitySchema('menu_link_content');
    +    $this->installEntitySchema('node');
    +    $this->installEntitySchema('taxonomy_term');
    +    $this->installSchema('book', ['book']);
    +    $this->installSchema('dblog', ['watchdog']);
    +    $this->installSchema('forum', ['forum_index']);
    +    $this->installSchema('search', ['search_dataset']);
    +    $this->installSchema('system', ['sequences']);
    +    $this->installSchema('tracker', ['tracker_node', 'tracker_user']);
    

    Making the assumption these are connected to the trait, couldn't they be in a helper to help maintenance with less repetition and avoiding drift if some other content gets added?

heddn’s picture

  1. This interface serves a dual purpose. On destination plugins, it returns the highest entity id, regardless if it was migrated. On the ID map, it returns the ID of last migrated item across all bundles of an entity.

    Highwater can use ID, but it can just as easily use any other value like a timestamp.

  2. No, it serves a dual purpose. It isn't always a destination plugin. It is also implemented by id map.
  3. It is results, but we want to sort the results by the machine name so they look pretty on the UI. (I didn't write this section of code, so someone else can correct me)
  4. The difficulty is that we have to introduce this in a BC way. So Paragraphs and Commerce won't be ready to throw this at them. Plus, they don't really have a standard template setup in many cases yet. And as migrate maintainers, we also support those two big content entities, so we'll get them onboard when they are ready.
  5. This is so we have a label to print in UI. Without it we just have the plugin_id.
  6. This is because we need it for seeing if the destination is translatable. It is used by this patch.

And I ran out of time here, so I'll leave someone else to correct my comments and finish out the rest of questions.

maxocub’s picture

#201.8: What about the mouthful MigrateDrupalCreateTestContentTrait?

#201.9: Moved all the installEntitySchema() in a helper method of the trait, but left all the installSchema() in place since they are not related.

#201.7: Not sure what to do here. We added that to maintain BC. If we add something to the constructor, a lot of contrib tests are going to be broken since they'll have to mock more things. That's what we're trying to avoid. But if there's a way to avoid having this getter and not break contrib test, I don't know it yet.

But according to Drupal 8 backwards compatibility and internal API policy, this kind of constructor is considered @internal, so in theory we could break BC:

Constructors for service objects, plugins, and controllers
The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the first place. Constructors for value objects where a developer will be calling the constructor directly are excluded from this statement.
heddn’s picture

  1. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateDrupalCreateTestContentTrait.php
    @@ -0,0 +1,107 @@
    +namespace Drupal\Tests\migrate_drupal\Kernel;
    

    Can we move it out of the Kernel namespace since it seems we can also use it for Functional tests too?

  2. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateDrupalCreateTestContentTrait.php
    @@ -0,0 +1,107 @@
    +trait MigrateDrupalCreateTestContentTrait {
    

    Some thoughts on the naming (which is hard). MigrateDrupalCreateTestContentTrait repeats the namespace of migrate_drupal What about CreateTestContentEntitiesTrait

  3. #201.7 => I don't think injecting is that important. I'd much prefer to preserve BC. We've got bigger fish to fry in migrate-land than a more pure DI implementation. However, can we open a follow-up for us to discuss this point and add a TODO for it? That will unblock this here.
maxocub’s picture

  1. Yes, we should.
  2. I like CreateTestContentEntitiesTrait.
  3. I agree we should move the discussion to a follow-up and unblock this issue.
maxocub’s picture

Status: Needs work » Needs review
FileSize
55.54 KB
4.25 KB

Re #204:

  1. Done.
  2. Done.
  3. In fact, there's already a follow-up with a todo in the code for that. It's marked against Drupal 9, but we can move it back to 8 and continue the discussion there: #2919158: Add the MigrationPluginManager to Drupal\migrate\Plugin\migrate\id_map\Sql.
maxocub’s picture

Oups, previous patch will fail, I forgot to remove Kernel in the namespace, it's been a long day.

Also, I found out that we cannot put classes directly in the Drupal\Tests\migrate_drupal namespace so I put the trait in the Drupal\Tests\migrate_drupal\Traits namespace.

The last submitted patch, 206: 2876085-206.patch, failed testing. View results

heddn’s picture

Re #207 Trait namespace: I think I knew that. There's an open issue on it somewhere. I just don't know the nid off-hand.

Can anyone RTBC this thing? I think I'm disqualified since I've been fairly involved from the beginning. But +1 from me.

quietone’s picture

Status: Needs review » Needs work
FileSize
21.88 KB

Great job everyone!

Installed Welsh and Icelandic and used devel to add content, users, taxonomies, well everything really. My test db was the d7 test fixture. It worked as expected. Read through the issue again, taking note of items for follow ups and found that everything needing a follow up, including my own niggles, has a follow up. I considered if comments needed to be added for the questions in #201, and decided not is not necessary.

To be inclusive and show what this can do I recommend that this change record, When migrating from the UI, a warning is now displayed if content may be overwritten uses an image that includes translations. Such as the result of my testing, attached.

The other change record, New classes for auditing migrations for ID conflicts, is accurate but would benefit from a clearer section what contrib developers need to do to add auditing. For example, if my contrib module is doing a migration with destination: entity:xyzzy, what exactly do I need to do or add? Is adding audit: true sufficient?

I also found 2 nits, which don't prevent this going RTBC. However, I am setting to NW to get feedback on the comments about the change records.

Found 2 nits.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
    @@ -3,7 +3,12 @@
    +use Drupal\Core\StringTranslation\TranslatableMarkup;;
    

    Extra semicolon

  2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -153,6 +159,20 @@ public function testMigrateUpgrade() {
    +    $this->assertSession()->pageTextContains('Content may be overwritten');
    

    These could be changed to use
    $session = $this->assertSession();, which is defined later, after this new block of code.

maxocub’s picture

Thanks @quietone for the review!

  1. I added a new screen shot in When migrating from the UI, a warning is now displayed if content may be overwritten showing all the currently audited (core) entities and the warning about translations.
  2. In New classes for auditing migrations for ID conflicts, I move the part explaining how to enable auditing at the top and tried to make it clearer.
  3. I fixed the two nits.
  4. Also, I saw in your screen shot that the results were not well ordered, I also fixed that.
masipila’s picture

I had a biref chat with quietone on IRC and reviewed the change records for readability. I made some small updates to both of them.
https://www.drupal.org/node/2928118
https://www.drupal.org/node/2928113

I also created a documentation follow-up for adding the ID auditing examples to Migrate API documentation handbook, see #2929671: Add ID auditing to Migrate API handbook

Cheers,
Markus

quietone’s picture

Status: Needs review » Reviewed & tested by the community

Thank you maxocub!! Especially for the extra step of sorting the conflict results for the user. How kind of you.
Thank you masipila!! You stopped whatever you were doing and reviewed and updated the change records. And gave yourself a new issue to update documentation. Good call.

Oh my, everything has been addressed here. Awesome work from all on a fiddly problem

I have kept myself away from this issue, only doing review, not only because the problem is fiddly but because I could now do this.

Pause, drum roll.....

Setting to RTBC!

Committers, please include masipila.

larowlan’s picture

Issue tags: +Needs UX review

Tagging as needs ux review to get the text reviewed, I'll ping @yoroy to let him know.

Will review the patch either today (Sun) or tomorrow (Mon)

larowlan’s picture

Adding review credits

  • @masipila for reviews, issue summary updates, manual testing and change record updates :tada:
  • myself for reviewing
  • @krystalcode for significant documentation updates in this issue space :applause:
larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Couple of questions - glad to see we're getting close here

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
    @@ -94,6 +94,10 @@
    +      'label' => $storage->getEntityType()->getPluralLabel(),
    

    Nice, this is much cleaner than the previous time I reviewed

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
    @@ -78,4 +93,15 @@ public function getIds() {
    +    $query = $this->storage->getQuery()
    +      ->sort($this->getKey('revision'), 'DESC')
    +      ->range(0, 1);
    

    Should this call ->allRevisions() to ensure the highest ID considers all revisions, not just the default one (i.e. a draft forward revision)? If so, means we're missing test coverage for that scenario too.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -925,4 +928,69 @@ public function valid() {
    +   * @todo Inject as a dependency in https://www.drupal.org/node/2919158.
    ...
    +  protected function getMigrationPluginManager() {
    +    return \Drupal::service('plugin.manager.migration');
    

    This is a reasonable trade off, pragmatism++

  4. +++ b/core/modules/migrate/tests/src/Unit/Plugin/migrate/destination/EntityContentBaseTest.php
    @@ -51,6 +56,11 @@ protected function setUp() {
    +    $this->entityType->getPluralLabel()->willReturn('wonkiness');
    
    @@ -104,14 +114,11 @@ public function testImportEntityLoadFailure() {
    +    $this->entityType->getKey('id')->willReturn('id');
    
    +++ b/core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php
    @@ -48,6 +49,12 @@ protected function setUp() {
    +    $entity_type->getSingularLabel()->willReturn('crazy');
    +    $entity_type->getPluralLabel()->willReturn('craziness');
    

    :)

  5. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -457,6 +462,148 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +      if ($destination instanceof EntityContentBase && $destination->isTranslationDestination()) {
    ...
    +      elseif (!$result->passed()) {
    

    Is there a reason why we don't check $result->passed() for translations? If so - can we add a comment to that effect? If not and this is a bug, we're missing tests? I think its intentional because for translations we're just not sure (the warning message seems to indicate that), but a comment here to clarify why would help in the future.

maxocub’s picture

Thanks @larowlan for the review!

2. Good catch.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
@@ -78,4 +93,15 @@ public function getIds() {
+    $query = $this->storage->getQuery()
+      ->sort($this->getKey('revision'), 'DESC')
+      ->range(0, 1);

This code does not even do what we want. Instead of returning the highest revision ID, it's returning the entity ID of the highest revision ID.
I wrote a failing test to show this bug, but I haven't found a solution yet.

5. No, translations are not yet supported by the audit system, it will be done in this follow-up: #2905759: Before upgrading, audit for potential i18n ID conflicts. I added a comment in the code.

masipila’s picture

I updated the Known Issues page quite heavily for better readability (diff).

I have a question on the text that we're showing in the UI, see conflicts.png in #211.

We are using quite strong language when warning about the translations. The Known Issues page needs an example of this scenario, currently the examples don't say anything about translations.

Cheers,
Markus

larowlan’s picture

Something like this might work.

$values = $this->storage->getQuery()
  ->allRevisions()
  ->sort($this->getKey('revision'), 'DESC')
  ->range(0, 1)
  ->execute();
// The array keys are the revision IDs.
// The array contains only one entry, so we can use key().
return key($values);

When you call ::allRevisions in an entity query, the returned values are keyed by revision ID. But the values are still the entity IDs, so you can pass it along to ::loadMultiple - which is the typical use case.

masipila’s picture

Status: Needs review » Needs work

Re #218. I discussed this with maxocub in IRC.

23:22 < masipila> so what's so scary about the translations that we need to use this strong language?
23:23 < masipila> if we're warning explicitely about the translations, we should have something on this topic in
                  the handbook
23:23 < masipila> but I can't figure out a good example...
23:24 < maxocub> Translations are not audited yet. If you upgrade from the UI, it's not that big a deal because
                 it's a one shot upgrade
23:25 < maxocub> so if it's not on a clean Drupal 8 install, the nodes will be a audited if it's a translation or
                 not
23:26 < maxocub> but if you do incremetal migrations, that's another story
23:26 < maxocub> You could migrate d7_node, then add translation manualy on those migrated nodes, and then migrate
                 d7_node_translations.
23:27 < maxocub> Then you risk losing manualy created translations
23:27 < masipila> and that's it?
23:28 < maxocub> I think it is.
23:29 < masipila> I can add that example to the handbook but the wording in the warning is not very good IMO...
23:29 < masipila> I'm going to kick this back to NW

So how about changing the wording something like this (commenting here instead of providing a patch to save time).

Current warning
It looks like you are migrating translated content. Be extra cautious and make sure you aren't introducting any conflicts. For more on the subject, see the online handbook entry for handling migration conflicts.

Proposed warning
It looks like you are migrating translated content. The possible ID conflicts for translations are not automatically detected in the current version of Drupal. Refer to the upgrade handbook for instructions on how to avoid ID conflicts with translated content.

Cheers,
Markus

masipila’s picture

And now that we're anyway touching the patch, I propose some rewording also for the first warning.

+  protected function conflictsForm(array &$form, FormStateInterface $form_state, array $conflicts) {
+    $form['warning']['#markup'] .= '<p>' . $this->t('Upgrades work on clean and empty new installs of Drupal 8. However, it looks like you have content in your site that is unknown to the migrate system; perhaps manually added. These new entities <strong>may be overwritten</strong> if you run this upgrade. For more information, see the online handbook entry for <a target="_blank" href=":id-conflicts-handbook">handling migration conflicts</a>.', [':id-conflicts-handbook' => 'https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8#id_conflicts']) . '</p>';

Proposed wording
The upgrade should be performed on a clean Drupal 8 installation. It looks like you have content on your site which may be overwritten if you continue to run the upgrade. For more information, refer to the upgrade handbook.

--clips--
I also suggest to add a prefix 'WARNING' to the heading i.e. 'WARNING - Content may be overwritten'

maxocub’s picture

Status: Needs work » Needs review
FileSize
62.64 KB
5.56 KB

Great, I didn't know that about allRevisions(), this should fix the failing tests.

I also updated the warning messages as suggested by @masipila.

I'll update the screen shots later tonight or tomorrow.

The last submitted patch, 217: 2876085-216.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 222: 2876085-222.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review

Test failures seem to be in code unrelated to the patch. Try a retest.

quietone’s picture

FileSize
48.58 KB

Good, tests are passing. Here is a screenshot

edit: add comma

heddn’s picture

Leaving at NR but I think the only thing stopping RTBC again is the UX review. Am I right?

maxocub’s picture

Yes, the UX review is the only thing left (for now).

phenaproxima’s picture

Tagging for usability review (I believe "Needs UX review" is now considered defunct).

webchick’s picture

Great, we have UX calls on Tuesdays at 12:30pm Pacific time in #ux on Drupal Slack (what is that in my time zone?). Feel free to come and demo your patch and we can get you unblocked. :)

heddn’s picture

@webchick we discussed who on the team could be available and I'll be joining the call to present.

heddn’s picture

neclimdul’s picture

Status: Needs review » Needs work

Sorry for the review and the sudden disappearance. Sudden flu.

#201.7 => I don't think injecting is that important. I'd much prefer to preserve BC. We've got bigger fish to fry in migrate-land than a more pure DI implementation. However, can we open a follow-up for us to discuss this point and add a TODO for it? That will unblock this here.

Say no to complexity guys. This is an anti pattern we accepted to deal with hard BC rules but this is clearly not a BC, there isn't any pragmatism here. lets not dig a bigger hole and just do it right. If someone's code does breaks using an internal method, they move to the actual interface(::create) and life is good, nothing breaks for them again in d8 and they learn their lesson about contain object constructors.

yoroy’s picture

Apologies for this drive-by style comment in a long hard issue, but we're discussing this in depth in our upcoming UX call later today and I want to put in some thoughts on the texts:

  1. If we're adding the "Warning" word explicitly, should we then not set an actual message of type warning? "Existing content may be overwritten".
  2. Simpler list heading: "There is conflicting content of these types:"
  3. Simpler list heading: "There is translated content of these types:"
  4. @masipila did a great rewrite of the explanatory text already, but maybe we should still lead with the problem first, solution second: "It looks like you have content on your site which may be overwritten if you continue to run the upgrade. The upgrade should be performed on a clean Drupal 8 installation. For more information, see the upgrade handbook."
heddn’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
47.03 KB
3.92 KB
62.31 KB

re #233, but sure to comment over in #2919158: Add the MigrationPluginManager to Drupal\migrate\Plugin\migrate\id_map\Sql to that effect.

re #234: I've addressed each of your points in this latest patch. Screenshot added to IS.

masipila’s picture

#234: I love these improvements, thanks yoroy! And thanks heddn for taking the time to prepare this for the UX call!

@yoroy, I'll comment on the general usability issue #2773205: Come up with a design for highly destructive operations in confirm forms, hopefully you'll notice the comment before the call today.

Cheers,
Markus

Status: Needs review » Needs work

The last submitted patch, 235: 2876085-235.patch, failed testing. View results

maxocub’s picture

Status: Needs work » Needs review
FileSize
62.39 KB
4.47 KB

This should fix the failing tests.

yoroy’s picture

Status: Needs review » Needs work
FileSize
204.97 KB

Notes from our review during the UX call:

- In the first screen we establish definitions for old site, new site. We should incorporate those here to be as clear as possible about where the conflicting items live (in the warning message and in the explanatory paragraphs, maybe even in the two sub headings?)
- Wording of the button: replace comma with full stop or semi-colon, not sure what we ended up on.
- Reshuffle the text so that the explanatory paragraphs are under their respective heading. We considered Heading, Paragraph, List and Heading, List, Paragraph. Although the first option seemed to be "nicer", the latter is actually better: we don't have to rewrite the current headers, don't have to introduce another header to define what's in the list and by ending with the paragraph we end with the links for more information in handbook pages so that's a good thing to end on:

So I think we should go with the latter: Heading, List, Paragraph.

maxocub’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
73.52 KB
62.64 KB
3.38 KB

Thanks for reviewing this during your UX call!

Here's the updated patch and a screenshot.

maxocub’s picture

Oups, I forgot to modify the warning message and the button text. Will do in an upcoming patch.

maxocub’s picture

  • Added ' on your new site.' to the warning message.
  • Used a hard stop in the button text.

Status: Needs review » Needs work

The last submitted patch, 242: 2876085-242.patch, failed testing. View results

maxocub’s picture

heddn’s picture

Minor wording change on the i18n warning to incorporate the language of old site.

- It looks like you are migrating translated content.
+ It looks like you are migrating translated content from your old site.

The last submitted patch, 244: 2876085-244.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 245: 2876085-245.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review

Ooo, the testbot is back. The errors look unrelated to the work at hand. Let's retest.

quietone’s picture

Status: Needs review » Needs work

The last items here were about not using DI (see #233) and then engaging with and responding to a usability review.

For the former, changing to use dependency injection for the MigrationPluginManager will done in 9.x #2919158: Add the MigrationPluginManager to Drupal\migrate\Plugin\migrate\id_map\Sql. For the latter, the result of the usability review (#239) has been implemented, tests updated accordingly and there is consistent wording on the new form and the upgrade form when referring to each site. Thanks yoroy helping to get this right and providing the summary.

However, this change record needs a new screenshot.

masipila’s picture

Status: Needs work » Needs review

I have updated the Change Record to contain the latest screenshot. https://www.drupal.org/node/2928113

quietone’s picture

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

Great thanks, masipila. Since you beat me to uploading a screenshot to the CR I've added the one I made to the IS.

Back to RTBC.

neclimdul’s picture

Status: Reviewed & tested by the community » Needs work

I don't think this is RTBC, lets slow down.

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -16,6 +16,11 @@
+ *
+ * Configuration options:
+ * - audit: If set to TRUE, the migration's IDs will be audited. This means
+ *   that if the highest destination ID is greater than the highest source ID,
+ *   a warning will be displayed that entities might be overwritten.

This is still incorrectly done. It needs to be an annotation attribute.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

This is still incorrectly done. It needs to be an annotation attribute.

EDIT: After discussing with @neclimdul on IRC, I realized that what is being asked here is for the 'audit' property to be defined as a canonical part of the migration's plugin definition. Normally, this would be done by adding the property to the annotation class for migrations; however, migration plugins are entirely YAML-based (for backwards compatibility reasons) and therefore have no annotation class. One determines a migration's "auditability" thusly:

$migration->getPluginDefinition()['audit']

That's certainly not the most optimal thing in the world, but I don't think we should block this critical issue on that. I propose we open a follow-up, then, to add an isAuditable() method to MigrationInterface (along with a base implementation in Migration), in order to seal the abstraction correctly.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs followup
FileSize
62.75 KB
1.33 KB

Discussed further with @neclimdul on IRC and we arrived at a compromise.

This patch moves the $audit property, and its associated documentation, to the Migration class itself. We will open a follow-up to add a new isAuditable() method to MigrationInterface, which will seal the abstraction.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

Perfect. Thanks for sticking out and listening even though I was using the wrong word to get my meaning across.

For committers this is making a concrete property for something that's already happening in the constructor and consistently documenting it. The part that might seem missing is that existing constructor code but its there.

phenaproxima’s picture

phenaproxima’s picture

Added a @TODO pointing to the follow-up, at @heddn's request. Leaving at RTBC because all I did was add a comment.

heddn’s picture

+1 on RTBC!

The last submitted patch, 245: 2876085-245.patch, failed testing. View results

catch’s picture

Status: Reviewed & tested by the community » Needs work

Patch looks good. I think we need to leave the meta issue open (if not blocking) to keep looking at ways to stop people getting into this situation i the first place (like set autoincrement).

Did find two issues though, small but important so CNW for that:

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -294,4 +292,15 @@ protected function getDefinitionFromEntity($key) {
    +  public function getHighestId() {
    +    $values = $this->storage->getQuery()
    +      ->sort($this->getKey('id'), 'DESC')
    +      ->range(0, 1)
    +      ->execute();
    

    This needs an ->accessCheck(FALSE) - if the latest node is hidden by node grants we might not get it.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
    @@ -78,4 +93,18 @@ public function getIds() {
    +  public function getHighestId() {
    +    $values = $this->storage->getQuery()
    +      ->allRevisions()
    +      ->sort($this->getKey('revision'), 'DESC')
    +      ->range(0, 1)
    +      ->execute();
    +    // The array keys are the revision IDs.
    +    // The array contains only one entry, so we can use key().
    +    return (int) key($values);
    +  }
    

    So does this.

maxocub’s picture

Status: Needs work » Needs review
FileSize
62.68 KB
1.19 KB

Added the accessCheck().

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks, @maxocub! Can we also add a test of the access check, just to cover our asses? I figure that we can modify the existing tests to create at least one inaccessible piece of content (along with the accessible pieces of content), then ensure that it's considered during the audit...

EDIT: @catch said this to me in Slack, and it may prove useful for testing this: "it should be possible to re-use a test entity grants module for it"

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.74 KB
64.21 KB

Here's some tests.

I used the node_test module which implements hook_node_grants() and hook_node_access_records(), I hope that's what was meant by "it should be possible to re-use a test entity grants module for it".

The test-only patch should prove that the bug was real and that the fix works.

No interdiff since the test-only patch is an interdiff (except for the commented fix).

The last submitted patch, 263: 2876085-263-test-only.patch, failed testing. View results

maxocub’s picture

Oups, this test-only patch is just wrong. Let's try the right way.

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

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

We’re good!

larowlan’s picture

larowlan’s picture

And @catch

  • larowlan committed 5ea62f1 on 8.5.x
    Issue #2876085 by heddn, maxocub, phenaproxima, Jo Fitzgerald, vasi,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 5ea62f1 and pushed to 8.5.x.

Published the change record

Unpostoponed the follow up

Thanks everyone for the continued efforts here

heddn’s picture

I feel this is appropriate,happy dance. Thanks everyone for such a nice early Christmas present.

Status: Fixed » Closed (fixed)

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