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.
2021-07-01 - 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.
Add your items here.
| benjifisher |
#2571235: [regression] Roles should depend on objects that are building the granted permissions (edited) |
| benjifisher |
@alexpott wants to get this issue done before #2953111: Only migrate role permissions that exist on the destination |
| alison |
I know it was just last week, but I totally forgot about the migration aspect of this issue... Thank you for bringing it up! |
| benjifisher |
I am reviewing the active issue. Only tests left to review, how hard can it be? :wink: |
| benjifisher |
That issue makes it so that any time you save a user role with bogus permissions, you will get a deprecation notice. It will turn into an exception in D10. But there is an ... exception ... for migrations: no deprecation notice, ... |
| benjifisher |
I think when we return to the postponed issue, we should make it so that migrations log a message when saving bogus permissions. Maybe provide an option (as in the current patch) to delete them instead.Always log, option to delete. |
| benjifisher |
Then we have to decide what we will do in D10. |
| benjifisher |
#3210257: Improve migrate messages retrieval |
| marvil07 |
I ended up here in the context of drush mmsg --idlist implementation.For now the drush implementation is just skipping non-requested values, but it would be great to have an api to retrieve a set of items' messages, instead of all. |
| benjifisher |
E.g. for a source that has two source identifier keys like k1 and k2, where only three id map messages are wanted to be retrieved, the call would be similar to $id_map->getMessagesSubset([ ['k1' => 1, 'k2' => 1], ['k1' => 2, 'k2' => 1] ]). |
| benjifisher |
That would be 2 messages, not 3, right? |
| marvil07 |
indeed :stuck_out_tongue: feel free to add an extra one on the issue summary. I was trying to use 3 to make it clear that number_of_keys != number_of_requested_source_items |
| benjifisher |
One option is to look at the second argument, when it is not empty, and see whether it is an array of arrays. This has the advantage of not changing the API, but it feels hacky to me.In other words, I think you are right to propose a new method. |
| benjifisher |
OTOH, I am not sure it is worth the effort. If I dodrush migrate:message my_migration --idlist=1,2,3,4,5,6,7then what is the performance difference betweengetting one message at a timegetting all messages and filteringgetting just the messages for those IDs in a new method? |
| marvil07 |
I think it depends on the set of data on the messages tables.E.g. In the normal case, using sql migrate ID map plugin, and with a messages table of 2k items, retriving and iterating over the set to show only some of them may have visible performance effect. |
| benjifisher |
may have visible performance effectNot good enough. If we are going to add to the API, we need to know we are solving a real problem, not a potential one.Adding to the API is not so bad when we have a base class corresponding to the interface. That is not the case here. |
| marvil07 |
I am sure of the performance impact, the may is about not having measured it clearly :sweat_smile: (edited) |
| benjifisher |
OTOH, we can add a public method to the class without adding it to the interface.If we do that, then when we use the new function we should either use the class (not the interface) for the type declaration or we should test method_exists() and provide a fallback implementation. I am not sure which is preferred. |
| marvil07 |
"getting one message at a time" => n queries + big memory usage if lots of them"getting all messages and filtering" => one query + big memory usage if lots of them (current approach in drush)"getting just the messages for those IDs in a new method?" => one query + a little memory as possible if lots of them |
| benjifisher |
Does the first option use more memory than the third? |
| marvil07 |
likely similar, n queries would be the difference, but right I would change the first as"getting one message at a time" => n queries + close to as litthel memory usagas possible if lots of messages |
| benjifisher |
If there are 10 or even 20 items, I want to know the difference between the first and third option. I understand that in principle the third option is more efficient, but I want to know how significant the difference is.If this is an edge case and it only saves 3 seconds when there are 20 items, then it is not worth it. |
| marvil07 |
the database rountrips are the main difference on the 1st I think, so dependening on that you get an x factor to multiply by the amount of messages requested |
| benjifisher |
In real life, with 20 items, is that 0.3s or 3s or 30s? |
| marvil07 |
:straight_ruler: measures pending :slightly_smiling_face:Not sure if 20 is a good reference number, consider a drupal to drupal migration of a normal site, and nodes may be in hundreds and each of those can produce multiple messages.Not sure how to get the "usual number of nodes in a drupal site" statistic :sweat_smile: |
| benjifisher |
But we are talking about --idlist=... on the command line. Why is the total number of nodes relevant? |
| marvil07 |
Right, I was wrongly thinking on the 2nd case.Indeed, the improvement would be noticiable if the idlist is large.The change is in the spirit of "let us retrieve messages more naturally with an included filter" instead of workign around it. :bulb: |
| benjifisher |
By popular demand. Thanks, @heddn! Cc @damienmckenna |
| benjifisher |
There are still some open MRs on DrupalSpoons. Can we get those wrapped up? For example, there are 5 open MRs here: https://gitlab.com/drupalspoons/migrate_tools/-/merge_requests |
| marvil07 |
Thanks so much for this! :thankful:It will help a lot to make contributions to those project more visible on the drupal.org credit system. |
| benjifisher |
It is also easier to move an issue from core to one of these modules or vice versa. |
| benjifisher |
There are 11 open MRs here: https://gitlab.com/drupalspoons/migrate_plus/-/merge_requests |
| dinarcon |
Some PRs might come from patches in Drupal.org, but there are some that were only created in DrupalSpoons indeed |
| marvil07 |
Right, I think I have added some MRs there that do not have an equivalend d.o issue in the last months. |
| heddn |
I tried to merge as many of them as were ready |
| heddn |
but some have feedback that needs addressed |
| quietone |
@heddn, thanks for moving the projects back. I recall you saying you liked some of the functionality that drupalspoons has. |
| Matroskeen |
On that note, I’d like to propose the following change to Migrate Plus, that I wanted to add to Drupal core :rainbow-sheep:#3195568: [PP-1] Support recursive discovery for migration plugins#comment-14070112 (edited) |
| benjifisher |
@chrisfromredfin gave a session at DrupalCon, and has sample code here (now with GPL): https://github.com/chrisfromredfin/dcon21-milb |
| benjifisher |
Session page: https://events.drupal.org/northamerica2021/sessions/migrating-layout-bui... |
| benjifisher |
I think we could add a process plugin based on that code.There is an issue to add a destination plugin for LB, but I think that a process plugin is what we really want. |
| marvil07 |
One alternative path could be to create a destination plugin for Section and SectionComponent, since they are not entities.With that I think that it may be possible to do the rest via normal yaml migrations. |
| marvil07 |
e.g. block content creation could be just another migration |
| benjifisher |
What sort of source are you thinking of? Beware the 1-to-many problem.Any reason not to start with the process plugin? |
| marvil07 |
the use case he presented, i.e. content in html into one section at layout builder.Going with the destination feels more natural, and also tries to avoid the 1 source to N destination problem at the cost of more migration YAMLs |
| benjifisher |
Even if you have a destination plugin for Section and SectionComponent, you still need a process plugin for the Layout field, right? |
| marvil07 |
could it be constructed as in the paragraphs case? |
| benjifisher |
Please explain what you mean. |
| marvil07 |
The output at https://github.com/chrisfromredfin/dcon21-milb/blob/main/web/modules/cus... may be build manually, to produce the expected https://github.com/chrisfromredfin/dcon21-milb/blob/665e8f546f5f99a13253...…]36 |
| benjifisher |
So you think we do not need a process plugin, justprocess: layout_builder__layout: - plugin: get ...Or maybe callback plugin with callable: [Section:class, 'fromArray'] ? |
| marvil07 |
something similar to that, I have not implemented one yet :sweat_smile: but sounds highly similar to the paragraphs case, where the right array can be build as passed to the entity reference revisions field with the paragraph |
| benjifisher |
Like['target_id' => 123, 'target_revision_id' => 456] |
| benjifisher |
We need a proof of concept. |
| Matroskeen |
I’ll thrown in my two cents, because I was doing migration to Layout Builder recently.I decided to make it as destination plugin because of the following reasons:In my case, content blocks were created in a bunch of other migrations, so I had to go over those block migrations, do the lookup and attach blocks as layout builder components; I’m not sure if I could achieve this by using process plugin;I also wanted to have a rollback just for the layout builder sections;Is it possible that might have two plugins?process plugin for simple cases (i.e. migrating body content as layout builder component).destination plugin for more complex cases?But, even with the simple approach, we need to created content_block first.It means, the first migration should be:node -> content_blockAnd then another one would be:node -> layout builder sections |
dinarcon, marvil07, alison, quietone, anmolgoyal74, gaurav mahlawat, Kunal Kapoor, Matroskeen, benjifisher, tonytheferg, heddn
Comments
Comment #2
benjifisherLet's discuss #2953111: Only migrate role permissions that exist on the destination at this meeting.
In Comment #67 on that issue, @alexpott says,
Comment #3
benjifisherMigrating into Layout Builder: there is some code we could use as a starting point for a process plugin at https://github.com/chrisfromredfin/dcon21-milb. At my request, Chris added GPL to the repository.
Comment #12
quietone commentedComment #14
quietone commented