Hello all, it’s time for the biweekly migration subsystem meeting. The meeting will take place in slack in various threads
This meeting:
➤ Is for core migrate maintainers and developers and anybody else in the community with an interest in migrations
➤ Usually happens every second Thursday and alternates between 1400 and 2100 UTC.
➤ Is done on the #migration channel in Drupal Slack (see www.drupal.org/slack for information).
➤ Happens in threads, which you can follow to be notified of new replies even if you don’t comment in the thread. You may also join the meeting later and participate asynchronously!
➤ Has a public agenda anyone can add to. See the parent issue for an idea of the typical agenda.
➤*Transcript will be exported and posted* to the agenda issue. For anonymous comments, start with a :bust_in_silhouette: emoji. To take a comment or thread off the record, start with a :no_entry_sign: emoji.
| benjifisher |
#2796755: [PP-1] Deprecate Migration::set() |
| benjifisher |
@quietone and I have been discussing this issue. The proposed resolution changed in Comment #174:
I really do not think we should be adding setMigrationDependencies()
|
| benjifisher |
We are considering reverting #3262395: $migration_dependencies has inconsistent structure. We think that reverting it will make it easier to fix the main issue after that change. |
| quietone |
did we find a case where someone is using set('migration_dependencies') to override what is already there? That is, there are not adding to the list? |
| benjifisher |
I do not think so. I think I would have commented on that in #176, which is the last time I reviewed the contrib usage. |
| benjifisher |
Looking back at #154, where I postponed the issue on #3262395, I see
We should not need setMigrationDependencies().
I guess I am wrong that #174 was a change in the proposed resolution. |
| quietone |
Ah, we have agreement on that point. |
| quietone |
I think the other one was from #180, about removing a block of code. |
| quietone |
Which I think we agree is not a problem? |
| benjifisher |
I have to get to my day job. I will have another look after work. |
| quietone |
And I am going to sleep. :zzz: |
| benjifisher |
#3354122: Do not attempt to migrate Drupal 7 field instances for fields whose storage was not migrated |
| dinarcon |
As noted in the issue, the D6 field instance migration does a similar check. Wondering if D7 should be doing the same. |
| benjifisher |
From the issue description, it seems like a good idea. The Steps to reproduce (STR) need a little editing (Step 2). |
| benjifisher |
Did you check the history? How did we end up with the check in one but not the other? |
| dinarcon |
I looked at the history, yes. And I do not see any clear reason as to why it was not added there. So, I wanted to ask if it was intentional. |
| benjifisher |
Which issue added the check to the D6 version? (If that is what happened) |
| quietone |
Added in #2416765: Migrate Drupal 7 Field/Instance/View mode settings |
| quietone |
It was added in #14 from a merge fro m about issue. |
| quietone |
But it looks like an oversight. |
| quietone |
The failing test needs to be fixed. |
| dinarcon |
D6 was added in #2121299: Migrate in Core: Drupal 6 to Drupal 8 |
| dinarcon |
D7 was added in #2416765: Migrate Drupal 7 Field/Instance/View mode settings |
| dinarcon |
Oh, Vicky had added the D6 link already. I was looking for the issues :sweat_smile: |
| quietone |
I doubt that a change is needed to MigrateFieldInstance test. |
| benjifisher |
Failed asserting that actual size 0 matches expected size 8.
Assuming that the test failure is simpoly related to the change, if we add field storage to make 4 of the 8 migrations work, then fix the expected numbers, I do not think we will need to add any new tests. |
| benjifisher |
I doubt that a change is needed to MigrateFieldInstance test.
Do you mean we can get that test to pass without changing it? :thinking_face: |
| dinarcon |
I updated the issue summary with hopefully a better step 2 for reproducing the issue and links to where D6 and D7 functionality was introduced. (edited) |
| benjifisher |
Do you need help dealing with the tests? |
| dinarcon |
I think I can do what you suggested. Adding 4 field storage. Let’s see how it goes :sweat_smile: |
| benjifisher |
// For each text field instances that were skipped, there should be a log
// message with the required steps to fix this.
$migration = $this->getMigration('d7_field_instance');
$errors = array_map(function ($message) {
return $message->message;
}, iterator_to_array($migration->getIdMap()->getMessages()));
$this->assertCount(8, $errors);
|
| benjifisher |
Earlier comment (in the test):
// All text, text_long and text_with_summary field instances using a field
// base that has both plain text and filtered text instances should not have
// been migrated.
|
| benjifisher |
Maybe this is an intentional difference between D6 and D7? From the test, it looks as though we want to get errors, with messages pointing to https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d... |
| quietone |
Oh, ignore my comment about the test. It is wrong. Somehow I thought a different test was failing. |
| quietone |
Ah, no this should not be changed. Looking for the issue |
| quietone |
#2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted) |
| quietone |
I recall that those are allowed to fail so that there is an error message to help the dev. |
| benjifisher |
Maybe we can repurpose the issue to add a code comment explaining why there is this difference between D6 and D7. |
| quietone |
Do you mean in d7_field_instance.yml ? |
| benjifisher |
Yes. |
| quietone |
I agree, that is a good idea. |
| dinarcon |
I was doing some testing the other day and I think I got to see the error pointing to documentation page even after applying the patch locally. Things are a bit fuzzy at the moment. I will revisit this later. Thanks for sharing # 2842222. I will look into it. |
| dinarcon |
So, the expected behaviour is that the field storage would be migrated, but the instances are not because of text processing settings? If I understand things correctly, this will also mane the configuration for formatters/widgets fail as well. Do we want to keep a field storage that has no field instance? (edited) |
Comments
Comment #2
benjifisherComment #5
benjifisherComment #6
benjifisherComment #7
smustgrave commentedWent into #migration channel for 4/20/2023. All conversation was captured and those that contributed to discussion were credited.