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.

Core migration issues

Next video meeting 2023-07-06 2100Z (tentative)

0️⃣ Who is here today?

quietone Hi
benjifisher Hello! You are up late!
quietone Just eager to make progress on that albatross around my neck
dinarcon Hello :wave:

1️⃣ What should we talk about today? Suggest topics here and I will add threads. I will also check for comments on the issue for today's meeting.

quietone #2953589: add example of how to use default_value with a lookup
dinarcon Can we have a look at #3354122: Do not attempt to migrate Drupal 7 field instances for fields whose storage was not migrated ?

2️⃣ Action items. To be added later.

3️⃣ Statistics

benjifisher Fixed in the last 2 weeks: 1 (not counting issues for meetings).
benjifisher RTBC: 2, 1 of which is Major.
benjifisher NR: 2, none Major and both have not been updated in more than one month.
benjifisher Needs subsystem maintainer review: 9
benjifisher Google sheet for recording stats: https://docs.google.com/spreadsheets/d/1o0Rjlc1vnnLP5bM5P-SMMyGzqn7258hi...

4️⃣ Comment in this thread if you are looking for ways to help. Give us some idea of what you would like to do: documentation, code review, testing, project management, ...

5️⃣ Previous minutes.

benjifisher We need a recording for last week's video meeting and a transcript for the 2023-04-06 meeting.

6️⃣ Announcements

benjifisher Not many people have been participating in these meetings, so we have decided to switch to biweekly. We will still alternate between 14:00 UTC (like today's meeting) and 21:00 UTC (next meeting on 2023-05-04).

7️⃣ Consolidate code in the `migrate_drupal` module. (edited) 

8️⃣ Deprecate `Migration::set()`

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:

9️⃣ add example of how to use default_value with a lookup

benjifisher #2953589: add example of how to use default_value with a lookup
quietone I have not been able to convince myself this is needed.
benjifisher I just added a comment to the issue. Short and simple:

+1 for won't fix.

quietone Phew!

1️⃣0️⃣ Do not attempt to migrate Drupal 7 field instances for fields whose storage was not migrated

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)

Thanks for participating! I will update 2️⃣. Please continue to add comments in the threads. In 1-7 days, we will post a transcript for today's meeting.

Comments

benjifisher created an issue. See original summary.

benjifisher’s picture

Issue summary: View changes

benjifisher’s picture

Issue summary: View changes
benjifisher’s picture

Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Fixed

Went into #migration channel for 4/20/2023. All conversation was captured and those that contributed to discussion were credited.

Status: Fixed » Closed (fixed)

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