[meeting] Migrate Meeting 2020-10-15 [#3175894]

Hello all, it’s time for the weekly migration initiative 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 here: https://www.drupal.org/project/drupal/issues/3175894. 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:

0️⃣ Who is here today? Is there something in Drupal 9.1 that has you excited?

dinarcon	Hello :wave: Olivero and Claro as new default themes.
Gábor Hojtsy (he/him)	Olivero for sure :slightly_smiling_face: also more multilingual migrations landing is great to see, thanks all for your work
damienmckenna	Looking forward to some of the improvements in the Migrate system.
heddn	lazy load of images!
mikelutz (he/him)	Here, but a little late today.
irinaz	Hi, I would like to invite those who are interested to a BoF at BADCamp tomorrow at 11 Pacific time for Migrate Feeds UI BoF https://hopin.to/events/badcamp-2020-sessions
alison	Alison here, catching up (specifically on documentation updates)while on a quick break during an awesome Migrate API training with @dinarcon at Virtual BADCamp Nope, but I'm excited for y'all's excitement :dog_excitement:
quietone	Hi. I just wan t to finish the multilingual migrate meta
benjifisher	IIUC, Olivero is going in as experimental, not the default theme. I am not sure about Claro, but it might be the default in Umami before it becomes the default in Standard.

1️⃣ What should we talk about today? Suggest topics here and I will add threads.

Gábor Hojtsy (he/him)	Remaining issues re multilingual, see #2208401: [META] Remaining multilingual migration paths#comment-13860863
heddn	#3177118: getRequirements missing from D9
benjifisher	Sorry, I missed these.
Gábor Hojtsy (he/him)	np

2️⃣ Action items. To be added later.

benjifisher	NR: #2925899: MigrateUpgradeImportBatch does not use source_private_file_path & source_base_path correctly, making it impossible to have public & private files in separate locations. @benjifisher
benjifisher	Add https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins/ch... to the menu so that people can find it. @benjifisher
benjifisher	Mark #3006750: Move memory management from MigrateExecutable to an event subscriber as postponed on #3000050: Replace \Drupal\Component\Utility\Environment::checkMemoryLimit with a PHP memory checking helper class and service. @benjifisher
benjifisher	Comment on #3095237: Migrate Drupal 7 date field "todate" value and set to NW. @heddn and @benjifisher (done)
benjifisher	Update #3150949: Add a migration source plugin for JSON:API. @benjifisher (done)

3️⃣ Statistics

benjifisher	Fixed since last week's meeting: 2 (not counting meeting issues)
benjifisher	RTBC: 6, all Normal priority. Two have been waiting for more than a month.
benjifisher	NR: 24, including 5 Major. One (Major) issue has been waiting for a little more than a month.
benjifisher	I added that issue to 2️⃣.

4️⃣ Documentation

benjifisher	I am now a maintainer of the guides for the Migrate API. Thanks, @masipila!
benjifisher	New page: https://www.drupal.org/docs/drupal-apis/migrate-api/process-pipelines (by me). There is just one example so far, but it is a start. I wonder if I should move it to the sub-guide on process plugins, or rearrange the menu of the main guide.
benjifisher	New page: https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins/ch.... Thanks, @droces! (maybe not on Slack?) It is not yet in the menu, so basically impossible to find without a link. IMO it overuses bold and italic a little.
benjifisher	Reviews and updates are welcome. Documentation is a team activity!
benjifisher	@damienmckenna: Can you remind me of your plans for https://www.drupal.org/docs/drupal-apis/migrate-api/migrate-field-plugins? It is a sub-guide, and so far there is just one (overview) page. Maybe start by creating some stub pages? That will make it easier for others to contribute. Also, that is the only sub-guide that I do not have permissions to administer. Can you add me as a co-maintainer?
damienmckenna	I've added you.
damienmckenna	For the field plugins pages, I was hoping to have the first page be an overview, then provide an annotated example in a separate page; the initial page has kinda turned into a brain dump that needs reorganizing.
alison	Awesome new pages, Benji, thank you!

5️⃣ Move memory management from MigrateExecutable to an event subscriber

benjifisher	#3006750: Move memory management from MigrateExecutable to an event subscriber (edited)
benjifisher	Previously, I argued for some changes to make the new MemoryManager class more testable. I was outvoted. While reviewing the latest patch, I still think I was right, but now I have new reasons to recommend the restructuring. Let me see if I can change your mind, @heddn, @quietone, @mikelutz (he/him).
benjifisher	When we run low on memory, ...The MemoryManager class calls getMemoryUsageInBytes() (a wrapper for memory_get_usage()), saves the value in an event, and dispatches the event.The one event listener in the patch does not look at the stored usage value.If another module implements an event listener, it gets the stored value even though our event listener (or others) may have succeeded in reducing usage.
benjifisher	I think it makes more sense to let each event listener get the current memory usage, not whatever it was when the event was created and dispatched.
benjifisher	To do that, create a new class with the getMemoryUsageInBytes() and getMemoryLimit() methods (public). Save an object of that class in the event, instead of the current values. Then the event listener can check $event->getClassName()->getMemoryUsageInBytes() inistead of $event->getMemoryUsageInBytes() .
benjifisher	Instead of making a new class, we might add these methods to the existing Drupal\Component\Utility\Environment.
heddn	@benjifisher there's also a more global drupal issue for memory management. perhaps some of this could be resolved there?
benjifisher	This strategy gives event listeners current information, and it also makes it easier to test the MemoryManager class, as I said in my previous reviews. We just mock the Environment (or newly added) class and injuect that into the MemoryManager class when we test.
benjifisher	Link?
benjifisher	Anyway, it seems to me that we should not delegate this to a follow-up issue because once we add an event dispatcher, any changes would have to involve deprecating the old API. I want to get it right the first time.
heddn	well, maybe it isn't 100% what I was thinking, but #3000050: Replace \Drupal\Component\Utility\Environment::checkMemoryLimit with a PHP memory checking helper class and service is linked as a related issue
benjifisher	I missed that! Yes, I think that is the way to go.
benjifisher	The current patch already has a method that wraps memory_get_usage(), which is not in the original Environment class. IMO, we should postpone our issue on this one. Any other votes?
heddn	postpone!
benjifisher	I see that @mikelutz (he/him) already commented: #3000050-23.

6️⃣ Migrate Drupal 7 date field "todate" value (edited)

benjifisher	#3095237: Migrate Drupal 7 date field "todate" value
benjifisher	I know next to nothing about field plugins, so help me out here! @heddn's latest comment:If we say the destination is datetime_range, but leave the plugin in datetime, that is going to confuse things. But we can't move it either, because of sites that didn't know they need datetime_range and don't have it installed. Not sure what to suggest/do here.
heddn	@benjifisher the destination module is used on field plugins to display the nice review form in migrate drupal ui. its what let you know if all your data is migrated
benjifisher	OK, I was about to ask what the effect of `destination_module` is.
benjifisher	It seems to me that there are bigger problems if datetime is enabled but not datetime_range. (edited)
benjifisher	Could we be setting fields to use a type from a module that is not enabled? That seems serious.
heddn	@benjifisher the 2 modules are distinct. Yes, we have problems. The field plugin really should be broken up into 2 plugins for this reason. But that's hard because there isn't a good way to know when to use one field plugin vs the other.
benjifisher	You mean, there is no good way to know until we start migrating entities that use the field, right?
benjifisher	It does seem complicated, but I think it is necessary. Should we mark the issue NW? (I think so.)
benjifisher	Can we say that when datetime_range is enabled, we use its field plugin? There must be some sort of priority or weight that determines which plugin is used. I hope. Then that plugin can extend the one from datetime or maybe pass, letting the next plugin take over.
heddn	@benjifisher not a bad idea. plugins have alter hooks. we could have datetime_range override the one from datetime.
heddn	and we should throw errors/exceptions if we try to stuff range data into datetime.
benjifisher	We have to do something if datetime_range is not enabled. At least log a migration message. Maybe throw an exception.
benjifisher	Can you add some of this to the issue and set it back to NW?

7️⃣ Add a migration source plugin for JSON:API

benjifisher	#3150949: Add a migration source plugin for JSON:API
benjifisher	He did not change the status to NW, but Wim Leers left a review with several comments that look useful (no surprise).
benjifisher	I looked at the JSON files in tests/data/. Are missing_properties.json and recipes-data.json copied from Migrate Plus?We have to adjust the URLs in the links section. Currently, they all start http://d9b.ddev.site:59006/. I am not sure what the right way to indicate relative file links is, but we should change them to something like file:./recipes_0.json and file:./recipes_1.json. Alternatively, I think that ./recipes_0.json and ./recipes_1.json are valid in JSON:API 1.1 (currently RC status). And recipes_1.json should not have a next link.
benjifisher	I asked about this on the #contenta channel and got some replies. I will comment on the issue.

8️⃣ [META] Remaining multilingual migration paths

benjifisher	#2208401: [META] Remaining multilingual migration paths
Gábor Hojtsy (he/him)	It looks like that got unblocked (edited)
benjifisher	There are a few child issues that have been committed to 9.1 and are waiting for backports to 9.0. Once those are resolved (either committed to 9.0 or not) I think we can close the meta. Do we need to pay attention to the meta now?
Gábor Hojtsy (he/him)	I don’t know what exactly is left though
Gábor Hojtsy (he/him)	I think @quietone wrote that @wimleers (he/him) pointed out that there are still outstanding todos with all of those committed still?
benjifisher	#3008028 is postponed on #3112249, which is postponed on #3110669, which is RTBC.
benjifisher	... there are todos in core referring to this issue.I think that means that there are @todo comments in core.
benjifisher	$ ag -ri 2208401 corecore/modules/config_translation/migrations/state/config_translation.migrate_drupal.yml13: # See #2208401: [META] Remaining multilingual migration paths
benjifisher	We should not close the meta issue without removing these comments. I guess.
Gábor Hojtsy (he/him)	Yup
quietone	I want to check the state of the translation of user profiles before making a decision on closing the meta. Working on this is my top priority.
Gábor Hojtsy (he/him)	Thank you @quietone
quietone	I've done my research and closed the Meta! #2208401: [META] Remaining multilingual migration paths
benjifisher	@quietone: What about the # See #2208401: [META] Remaining multilingual migration paths comments in the YAML files?
quietone	That is in the followup, #3175729: Mark i18n migrations as finished

9️⃣ getRequirements missing from D9

benjifisher	#3177118: getRequirements missing from D9
benjifisher	Don't we already have an issue for this? @neclimdul was trying to get that in. Or am I thinking of something else?
heddn	@benjifisher I thought the same. In which case, we can close duplicate. I couldn't find it though :disappointed:
heddn	It doesn't seem super complicated, but I could be wrong
benjifisher	Fixed: #3154398: Migrations don't have an accessor for requirements
benjifisher	Another reason to look forward to 9.1. :wink:

1️⃣0️⃣ 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.
quietone	@benjifisher thanks for facilitating.

Participants:

dinarcon, Gábor Hojtsy, damienmckenna, heddn, mikelutz (he/him), irinaz, alison, quietone, benjifisher