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 over chat.
➤ 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: `#3067311: [meeting] Migrate Meeting 2019-07-11`
➤*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.
| mikelutz | Michael Lutz- Migration sub-system maintainer. |
| benjifisher | Benji Fisher - developer |
| alison | Alison "Cornell DrupalCamp is 9/26-9/27" McCauley -- migrater, lurker, community member, developer, user of too many words |
| benjifisher | I would like to discuss two issues: - #2958672: Use migration lookup on text fields - #2809433: Migrate support for deleting items no longer in the incoming data |
| benjifisher | I guess I should post those on the :2: thread. |
| Gabor Hojtsy (he/him) | Gábor, core committer |
| phenaproxima (he/him) | Adam, Boston, lapsed Migrate maintainer but mostly lurking |
| heddn | heddn, migrate maintainer |
| andypost | Andy, contributor |
| Joshua Turton (srjosh) | Joshua Turton (srjosh) Phase 2 senior developer and migration specialist |
| woodseowl | Eric, developer, migration user, curious lurker |
| benjifisher | #2809433: Migrate support for deleting items no longer in the incoming data |
| benjifisher | #2958672: Use migration lookup on text fields |
| Gabor Hojtsy (he/him) | backport of #2936365: Migrate UI - allow modules to declare the state of their migrations (potentially) |
| alison | If migrate_plus is on the table, I'd love to get a feel for whether some "narrow down the source" plugins are appealing to maintainers.
I have arguments in favor ready to go :) Example plugins: |
| andypost | deprecation of migrations, faced again in #3022401: Remove useless config action.settings.recursion_limit |
| mikelutz | #3048413: Split iterable part of SourcePluginBase into a trait |
| mikelutz | I’m still not convinced this is necessary. I don’t see why you would make your own source that doesn’t extend the base, but still need the iterator stuff. The base plugin doesn’t make any assumptions that can’t easily be overridden. |
| mikelutz | @marvil07 |
| mikelutz | I’m +1 for closed - won’t fix. |
| phenaproxima (he/him) | I am also not in favor of this. |
| phenaproxima (he/him) | What would be more worthwhile, IMHO, is to force source plugins to become \Traversable, not \Iterator, and let them be iterable in any way they please. This would also open up a path for us to adopt generators. |
| benjifisher | Let me look up a related issue ... |
| benjifisher | #3054167: The getMessageIterator() method does not return an instance of Iterator |
| heddn | I’m also of the opinion of closing |
| mikelutz | Alright, marked as closed - won’t fix |
| benjifisher | No objection. I will tell Marco about the decision. |
| marvil07 | Well, I guess it is decided now :disappointed: I have created a related ticket following what @phenaproxima (he/him) suggested |
| marvil07 | BTW a detailed reasoning may be what I wrote on "On the implicit dependency on SourcePluginBase" at |
| mikelutz | #3051142: Remove deprecated MigrateUpgradeTestBase |
| phenaproxima (he/him) | +1 for removing deprecated shit! |
| heddn | if deprecated, we might need to keep it for contrib as it was a base class. are we triggering any deprecation errors? |
| mikelutz | It’s a test base class, so we don’t generally trigger deprecation errors on them or guarantee them. |
| mikelutz | We are free to remove it, and honestly if any contrib were using it, it would be commerce_migrate, so if @quietone is pushing to remove it, it’s fine. |
| mikelutz | imho* |
| andypost | contrib does not use it |
| mikelutz | Okay, RTBC |
| mikelutz | #3030937: Ensure language is not Null in translation source queries |
| mikelutz | I’m moving this to NW as there are some questions in the thread that need to be answered. |
| mikelutz | #3008028: Migrate D7 i18n menu links |
| Gabor Hojtsy (he/him) | posted a review at just now |
| mikelutz | Thanks! FYI, Vicky mentioned why this didn’t make it into the fixture in comment #30. Leaving NW for the rest of it. |
| Gabor Hojtsy (he/him) | there are still a LOT OF stuff that I am not sure we need in the fixture |
| mikelutz | Looking at the size, that’s probably true. |
| Gabor Hojtsy (he/him) | a few menu items for testing (based on the test) should not need a 700k patch :smile: |
| mikelutz | #3007102: Migrating to Date-only field does not drop time value |
| mikelutz | @damienmckenna |
| damienmckenna | Oh, sweet! |
| heddn | @damienmckenna if you would try out that patch, I think we have test coverage. Just need confirmation its fixed |
| damienmckenna | Will do. |
| mikelutz | #2745797: Add option to content entity destinations for validation |
| alison | Awesome concept, love it. What's the need at this point, need more reviews, or? |
| heddn | posted feedback to ^ |
| mikelutz | Set to NW per @heddn’s feedback. |
| mikelutz | #2979964: Migrate translations for D7 i18n taxonomy 'localized' terms |
| mikelutz | #3066277: Use the lock service for migration locks |
| mikelutz | @chx (he/him) @vijaycs85 |
| chx (he/him) | yes this is a lovely issue |
| mikelutz | I have concerns about the approach. |
| mikelutz | The migration status is used for a lot more than just locking, and that information seems to be thrown away in this patch. |
| mikelutz | #2936365: Migrate UI - allow modules to declare the state of their migrations |
| Gabor Hojtsy (he/him) | @webchick felt strong that backporting would be really useful so contribs can provide the yml and also actually get some value by providing it :slightly_smiling_face: |
| Gabor Hojtsy (he/him) | on the other hand release managers had questions about disruption possibilities which are not described in the issue or the change notice |
| heddn | i’m ambivalent. if someone does the CR updates and backports it, then go ahead. |
| Gabor Hojtsy (he/him) | IMHO collecting all the changes it makes to a running site would be best, ie. what behaves differently if the yml file is not in place |
| Gabor Hojtsy (he/him) | not worth the backport before a go-ahead from release managers |
| mikelutz | Again I feel it’s not as important to backport. Contrib can provide the yml now, they don’t have to wait. Given everything else going on, I won’t have time to do all the work needed to convince the RMs that it is backportable. |
| Gabor Hojtsy (he/him) | maybe @damienmckenna is interested to collect that info since he asked about the backport? |
| damienmckenna | The available time I have to work on things like this is going to be limited for a little while (moving! and too much billable work), but I'll try to get back to it soon (if someone doesn't beat me to it). |
| Gabor Hojtsy (he/him) | I added #3067333: Expose migration status on releases (and through drupal.org's API) for exposure on |
| mikelutz | #2809433: Migrate support for deleting items no longer in the incoming data |
| benjifisher | There is a working patch on the issue. @marvil07 and @quietone have worked on it; I have only tested and done not-picky reviews. (That is, I did not consider the big picture.) @heddn thinks there is a simpler approach. |
| benjifisher | Lucas wrote, > When this flag is set, it triggers the executable to loop over each source and destination ... How do you loop over the source (all items, without resetting it) unless there is a method on the source that gives you all items? A method to do that is included in the new interface, and implemented in the trait. |
| heddn | if we have to clone source/destination or make things messy to get it so we don’t have to tack an interface on things, that is the ideal approach |
| alison | I kinda skipped to the end of the thread, feel free to say "the answer is in the thread, read it" --
This functionality *feels* to me like it would get us a bit closer to something that folks could use instead of Feeds -- is that true, or totally ~left~ right-field here, or? |
| heddn | i also sense I’m being a little too picky about this. and hate that. but I also really don’t see why we need an interface for an 80% solution. and just adding a flag on the migration to make it syncable without having to add an interface in all kinds of places seems like a high bar for novice users |
| heddn | @alison agreed. it is very feeds like. and I want the feature. I just don’t like all the hacks folks need to do to make it work for out of the box sources and destinations. |
| benjifisher | @heddn: Would you like to add a comment to the issue suggesting that approach, or shall I do it? |
| marvil07 | @heddn Please notice that: • it was decided to move the issue out of core into migrate_tools 4 months ago, suggesting complexity. • the approach in the latest patch I posted is trying to work-around not being able to modify the `MigrateExecutable` base class. |
| marvil07 | So maybe we want to move back to the in-core approach? |
| marvil07 | I may me missunderstanding the hints, so please let me know. |
| heddn | @marvil07 there's a migrate tools `MigrateExecutable`. hack at it as much as you want :slightly_smiling_face: |
| marvil07 | @heddn the problem is that if not in core, we cannot do it without using custom sources from migrate_tools `MigrateExecutable`, which is the current approach. If we go back to the in-core approach, we can skip the problem of needing custom sources. |
| heddn | I seem to be missing something, for which I apologize :disappointed: |
| heddn | From my review though, it seemed very possible to loop over the entire source and destination, find things that aren't in either and delete them. |
| heddn | we could do that in migrate tools executable in a post import step. somewhere... anywhere |
| heddn | @marvil07 can't migrate tools' executable do any of this? post import? |
| marvil07 | @heddn I did not get into that approach, since it was suggested after my last patch, and would involve possibly many changes; so I am not sure if it is possible yet. I guess the next person trying to help that issue would want to try the suggested approach. |
| heddn | @marvil07 we have lots more flexibility in contrib and the goal of not altering source/destination is so enticing... I really want to see it happen that way. it will make feeds and any other use cases much more trivial |
| heddn | yes, we'll have an occasional one-off source and destination we need to optimize due to large data volume so we can do queries more efficiently, but for many of these syncs... we're talking 100 records. no need for a sledge hammer in that case |
| marvil07 | @heddn You are suggesting the following constrains: (c1) do not do it on core, i.e. do not modify the MigrateExeutable; and (c2) do it in a way that does not need to modify the source plugins. I think c1 and c2 are incompatible, but I may be wrong. c2 from migrate_tools is harder-than(if-possible) from core in the current approach. I guess the next person trying to help in that ticket may try the new suggested approach. |
| marvil07 | @heddn Also, in my experience 100 records is low, but I may be biased. |
| heddn | @marvil07 do not modify core. do modify migrate tools’ migrate executable |
| mikelutz | #2958672: Use migration lookup on text fields |
| benjifisher | The code here is all reviewed. The only remaining issue is whether to be more consistent with a related plugin in how the configuration parameters are named. Depending on which plugin we change, this may involve a change to something released in 8.x-4.2, so we have to deal with BC. |
| heddn | @benjifisher migrate_plus 4.x is receiving minimal support at this point. it has deprecations against D9, so I don’t plan to put a lot of effort into it. 5.x is the wave of the future |
| benjifisher | So BC is not a deal breaker. I am also willing to add code so that either parameter name is accepted, and declare one of them as deprecated. Just let me know which way to go. |
| heddn | target 5.x. i’ll release a stable once this lands |
| heddn | is my suggestion |
| benjifisher | I am still not clear on what that means. The patch is compatible with either branch. Does "target 5.x" mean that I should go ahead and change the existing plugin (released in 4.2) to use `xpath` instead of `expression` as the parameter name? |
| benjifisher | @heddn ^ |
| marvil07 | @benjifisher On the choosing of the parameter name I pased it on `DOMXPath::query()` first parameter name, so we may want to keep it. I am not opposed to change it to `xpath`, but I think it is a good idea to follow php, since we are using the dom php extension |
| heddn | @benjifisher sorry for the un-clarity. 5.x is compatible w/ D9, as stated. Since it is a major version, we can do BC breaks. Including renaming xpath and expression. Instead of making things BC, let's just fix it in 5.x and release 5.0 instead |
| heddn | so, target 5.0 with any BC breaks. if there's enough clamor from the community we could backport those things to 4.x, but I don't plan to put a lot of effort into a 4.3 release. I'd much prefer only focusing on 5.0, 5.1, etc |
| benjifisher | Thanks, that is clear. |
| mikelutz | #3057685: Plugin to only migrate users with certain roles |
| mikelutz | #3064016: Allow menu_link source plugin to filter menu links by menu name(s) |
| mikelutz | #3063778: Source plugin to only pull in certain URL aliases |
| alison | Thanks! So --
These kinds of optional configs at the source level make things like `migrate-status` and `migrate-messages` more informative / better representations of what I'm doing. ~(I find it especially frustrating to have to sift through hundreds of migration messages that just say "thing not migrated b/c skipped on empty" (sic).)~ (EDIT: blerg, i got mixed up, i take back the mmsg part of what I'm saying, it was not accurate at all) |
| alison | I actually think the users one might be the best example, esp since `MenuLink` already defaults to only including custom menu items (i.e. it won't migrate the entire development/management/admin menus, it only migrates custom menus and custom(ized) menu links in non-custom menus) -- for users, if your site lets "anyone" create an account, I think it'd be a common use case to want to *not* migrate users in the authenticated role.
That said, these source plugin concepts seem small and simple and helpful, to me. I don't see a downside -- y'know, as someone who isn't a migrate_plus maintainer :wink: |
| heddn | I have no objections here. Keep bringing these ideas into the queue |
| alison | Cool beans, thank you! Appreciate the feedback! |
| benjifisher | I agree with the arguments for modifying the source plugin instead of skipping during the process phase. Did you mention that it can be a lot faster? |
| benjifisher | IMO the only question is whether to add options to existing source plugins or to create new ones. If the base plugins are in core, then adding options is not an option. (sorry) I have not looked at the examples above, but I wonder if we should consider (for example) a user-source-with-more-options source plugin instead of the proposed user-with-role-restriction plugin that is proposed. |
| alison | I didn't, great poinit!
The base plugins are indeed from core. Great idea re: user-source-with-more-options |
| alison | (as mentioned elsewhere -- fyi @xurizaemon @chriscalip ) |
| xurizaemon | Awesome @alison :slightly_smiling_face: coming in late, are you thinking you'll make those optional filters for the existing sources, or new sources (as currently proposed by patches, at least last I saw) |
| alison | Thanks! They'll be new source plugins b/c the base plugins are core, not MP (as explained by Benji a couple comments up) |
| xurizaemon | Ah, got it. That's a pity (am I right to presume api stability is why it's not an option @benjifisher?) - but no biggie! |
| xurizaemon | (a pity you wouldn't get it into core, and great news that it'll be available for others! thx alison) |
| benjifisher | I think the policy is that core only includes functionality needed for the migrations defined in core. |
| mikelutz | #3022401: Remove useless config action.settings.recursion_limit |
| mikelutz | #3039240: Create a way to declare a plugin as deprecated |
| mikelutz | We recently created a way to declare a library as deprecated in a yml file, and I feel that that is the correct approach here, but there are some technical differences. The biggest problem being that all migrations are loaded at once, so it’s tricky to figure out where to throw the error. |
| Gabor Hojtsy (he/him) | I don’t know where that is exactly but #3064017: Create a means to mark an asset library as deprecated in a *.libraries.yml file should show previous way to do it that already landed. |
| Gabor Hojtsy (he/him) | c lol like @mikelutz said |
| mikelutz | One topic per NR issue makes for a lot of topics, but we haven’t cleared the queue in a couple weeks, hopefully we won’t have as many next week. |
| Gabor Hojtsy (he/him) | yeah focusing on less would be better :smile: |
| mikelutz | I didn’t think we’d end up with this many topics, we usually just triage the issue queue at these meetings, but with this format we had a LOT more participation than usual (which is a very good thing) |
| benjifisher | Maybe remove the "why you are here" question from the "introduce yourself" thread. Or am I the only one who sees that as an invitation to paste in issues to the wrong thread? |
| benjifisher | FWIW my participation is not related to the format of the discussion. |
| alison | I feel like it might just be me, but I'll say it in case it turns out other ppl think so, too... I read the agenda but then forgot that the idea was to review NR issues -- so like, in one thread, I asked "what's needed to move forward" and it turned out that was an unnecessary question? (or maybe it wasn't unnecessary, maybe the "what's needed" was in fact different per issue)
idk if that made any sense...... |
| alison | I like the "why you are here" concept b/c it helped me know who's who -- but maybe there's a different phrasing, if it's confusing for folks |
| mikelutz | Good to know, my point was the hangouts often turned into just me and Lucas triaging the issue queue, so the extra participation here has been very welcome. |
| Gabor Hojtsy (he/him) | HUGE +1 to threads per issue, the one thread meeting was *impossible* to follow/participate in (previously) |
| alison | I'll echo what Benji said -- the format didn't / won't affect my participation, I was here today b/c I happened to be on Slack when the meeting time came around -- I didn't know the meeting existed before :slightly_smiling_face: I'll be putting it into my calendar going fwd, and join when I can. |
| mikelutz | Yeah, I strongly agree, this format is way better. While the format may not have directly affected your participation, what it did do was get your attention in the way that the one line hangout link probably never did. |
| mikelutz | Triaging the issue queue will be a part of this meeting, but if we as maintainers do a better job of that throughout the week, then it hopefully won’t be so overwhelming. There were issues in NR for 6 weeks we moved today. |
| heddn | love the format. lots of topics. we could bundle some of the i18n/multi-lingual together |
| heddn | I'm not sure that having that many topics is all that off-putting either. it just means we have a lot to discus |
Comments
Comment #3
mikelutzComment #5
mikelutzComment #7
mikelutzComment #9
mikelutzComment #11
mikelutzComment #13
mikelutzComment #15
mikelutzComment #17
mikelutzComment #19
mikelutzComment #21
mikelutzComment #23
mikelutzComment #24
mikelutzComment #25
mikelutzComment #26
mikelutzComment #27
mikelutz