Hello all, it’s time for the weekly 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 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-01-05 2100Z (tentative)

The hope is that most or all of the maintainers will attend. We will try to focus on longer-term goals than in the weekly meeting.

0️⃣ Who is here today?

mikelutz (he/him) I’m here, I swear!
benjifisher So do I: !@#$%^&*()
danflanagan8 I got sucked in

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.

2️⃣ Action items. To be added later.

3️⃣ Statistics

benjifisher Fixed since last week's meeting: 0 (not counting issues for meetings).
benjifisher RTBC: 1, which is Major.
benjifisher NR: 11, 2 that have not been updated in more than three months.
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, ...

Ryan Loos Not sure if this is the correct place for this, but I made my dom element replace plugin here and it could use some eyes

5️⃣ Previous minutes.

benjifisher - NR #3313383: Paragraphs Role Visibility (2022-10-13)
- NR #3312722: [meeting] Migrate Meeting 2022-09-29 2100Z (2022-09-29)
- NR #3308616: [meeting] Migrate Meeting 2022-09-15 1400Z (2022-09-15)
benjifisher Is there anything to follow up? Anything need to be changed in those minutes?
benjifisher Needs transcript: #3313877: [meeting] Migrate Meeting 2022-10-06 1400Z (2022-10-06)
benjifisher Now NR ^
benjifisher I reviewed the issue for 2022-09-15. Now Fixed.I added transcript and credits for all the other NR issues, so someone else should review them.

6️⃣ Announcements

7️⃣ static_map process plugin does not work with periods in keys

benjifisher #2827897: Update documentation for handling dots in the static_map plugin
benjifisher @danflanagan8 requests that one of the maintainers close this issue as Won't Fix. @mikelutz (he/him) has already looked at it and supports that. Of course, I could do it myself, but I would want to spend time reading all the comments first.
mikelutz (he/him) Did I say I support closing that?  I actually think that’s a valid use case we should handle.
benjifisher Am I thinking of the wrong issue?
danflanagan8 Neat! I’ll put that on my to-do list. Should be easy enough.
mikelutz (he/him) Possibly.  I tend to close things that I think should be closed. I recall that being brought up. I recall @danflanagan8 suggesting it should be closed, I recall looking at it and thinking it was a valid use case that isn’t supported currently due to yaml limitations and the way that plugin uses yaml keys in the map, and I recall not mentioning that I felt that way to anyone.
danflanagan8 It’s definitely a valid use case, but it just seemed like one of those things that core migrations work fine without.
benjifisher
I recall not mentioning that ...

How do you recall not doing something? :wink:

mikelutz (he/him) It’s going to be trickier than you think though.  static map already has a use case for map values as arrays, which is going to conflict with the proposed resolution at some level.
danflanagan8
It’s going to be trickier than you think though

bring it on :rolling_on_the_floor_laughing:

danflanagan8 I’ll approach it with humility
mikelutz (he/him) I would use map_key and map_value instead of key and value, since those are common words, I’d traverse the entire array recursively looking for any elements that are arrays containing exactly those two keys with a string for map_key, and replace them with the normal $key => value, then let the plugin run like normal. Need a CR that the nested array syntax is broken for those specific array values
benjifisher
... it just seemed like one of those things that core migrations work fine without

Yes, in order to limit scope we have a policy that "something" goes into core only if it supports core migrations. I do not think we hired a lawyer and wrote that down anywhere. But I think it applies to feature requests. This issue seems to me like a bug report, not a feature request.

mikelutz (he/him) Well, it’s a feature request because the plugin specifically documents that periods in keys aren’t supported. So it can’t be a bug if the plugin directly says you can’t do it.
benjifisher I think it would be safer, and maybe simpler, to add a top-level configuration key. The hard part is deciding what to call it.
benjifisher I know that an undocumented feature is a bug. TIL that a documented bug is a feature!
mikelutz (he/him) I’m not sure what we would have the config key do,  We want the nested structure and the optional [key => xx value => format] to work together as much as possible.
mikelutz (he/him) If you really needed to support a nested mapping that had exactly map_key and map_value as your second level somewhere I think that would be pretty crazy, but you could always add a third dummy mapping value to work around this new feature, as long as we confine our scan to arrays that only have those two keys and nothing else.
benjifisher Oh, the dots break even nested keys?
benjifisher So this does not work?
map:
  foo:
    foo.bar: foobar
    bar.foo: barfoo
benjifisher I guess that makes sense. Config objects are typically nested.
mikelutz (he/him) yes. YAML interprets dots as array indices. So that is the equivalent of
map:
  foo:
    foo
      bar: foobar
    bar
      foo: barfoo
mikelutz (he/him) And I think somewhere we error that yaml syntax out instead of allowing it to be interpreted like that.
benjifisher Is there some way of escaping the dots?
mikelutz (he/him) I don’t understand why quoting the keys with dots isn’t supported in yaml, but it isn’t.
danflanagan8 I’m pretty sure it’s just the config system complaining, not a yaml problem persé
benjifisher I think your YAML equivalent is missing some : characters.
benjifisher Yes, it is a problem with how the config system treats the dots.
danflanagan8 This is from symfony docs on yaml:
'symfony 1.0':
  PHP:    5.0
  Propel: 1.2
'symfony 1.2':
  PHP:    5.2
  Propel: 1.3
danflanagan8 Nothing special about dots in the keys there
mikelutz (he/him) hmmm. That might change my opinion…
mikelutz (he/him) Does this problem exist then if the migration yaml is defined as a core plugin, and not a migrate_plus config file?
benjifisher How about a configurable "escape sequence" for dots?
plugin: static_map
dot_escape: DOT
map:
  PCIe 3DOT0: pcie_3_0
  PCI: pci
benjifisher Let's start with a failing test. @danflanagan8, are you on it?
danflanagan8 Not immediately, but I’d be happy to write some tests
mikelutz (he/him) As I’m reading it, this error occurs on config import in migrate plus.. I don’t think any of the drupal code where we disallowed this would work on a migration plugin in core, and if I’m right, you won’t be able to write a test for this in core.
benjifisher If it is a problem in contrib (migrate_plus ) and can be fixed in contrib (migrate_conditions) then we close as Won't Fix, right?
benjifisher
... you won’t be able to write a test for this in core

Meaning you can write the test, but it will not fail, right?

mikelutz (he/him) I mean you can write a test that tests dots in keys, but since core doesn’t use the config system in the migration system, you can’t write a test that triggers the error that people are getting.
danflanagan8 I’m trying now In a meeting where I don’t have to talk.
mikelutz (he/him) If you are using migrate_plus, you don’t need a config key for a dot escape, you can wrap your map with the str_replace plugin to escape dots
danflanagan8 Right as usual, @mikelutz (he/him). This Unit test passes:
  /**
   * Tests when there is a dot in a map key.
   */
  public function testMapDotInKey() {
    $configuration['map']['foo.bar'] = 'baz';
    $this->plugin = new StaticMap($configuration, 'map', []);
    $value = $this->plugin->transform('foo.bar', $this->migrateExecutable, $this->row, 'destination_property');
    $this->assertSame('baz', $value);
  }
danflanagan8 And Kernel tests featuring static_map aren’t breaking when I add a dot to the map keys.
mikelutz (he/him) Not surprising, the issue isn’t with the migration system, it’s with the config system. You could write a test in migrate plus probably.
danflanagan8 The ball is in the maintainers’ court now: #2827897: Update documentation for handling dots in the static_map plugin#comment-14756508

8️⃣ SQL source plugins throw exceptions if database is not available

benjifisher #3312733: SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled
benjifisher We postponed #3063856: Add ability to view migrate_message table data on this new issue. There was some discussion in the 2022-09-15 meeting.
benjifisher It is a pretty simple fix to catch the exception, but we thought that it deserves a separate issue because it seems like a change that might have unintended consequences.

9️⃣ Wrap up

benjifisher 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
Status: Active » Needs review
benjifisher’s picture

Issue summary: View changes
quietone’s picture

I've compared the transcript to the slack thread and all looks good. The only thing is that I think mikelutz should have credit. @benjifisher, an oversight I presume?

benjifisher’s picture

@quietone: Yes, I am not sure how that happened. Thanks for checking!

quietone’s picture

Status: Needs review » Fixed

@benjifisher, thanks!

Status: Fixed » Closed (fixed)

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