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?
|
| 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
|
| 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:
|
| 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?
|
| 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:
|
| 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
Comment #3
benjifisherComment #4
benjifisherComment #5
quietone commentedI'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?
Comment #7
benjifisher@quietone: Yes, I am not sure how that happened. Thanks for checking!
Comment #8
quietone commented@benjifisher, thanks!