[meeting] Migrate Meeting 2019-12-19 [#3102231]

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 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 here.
➤*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.

0️⃣ Who is here today?

heddn	:raised_hand:
Nick Wilde (he/him) - DC 2020 DevOps Session Team	Nick, he/him, moderately here
quietone	I will be here soon
damienmckenna	Me.
webchick	Angie, albeit post-meeting. :)
alison	Hi! Catching up :)

1️⃣ What do we need to talk about? Mention your topics here, and we will start threads as needed.

heddn	What was your worst Christmas present ever? :slightly_smiling_face:
quietone	What ever it was I deleted it from my memory!
alison	I gave my then-boyfriend (now husband) a DVD one year and he gave me a fancy trip :laughing:

2️⃣ Multilingual!

mikelutz	#3076447: Migrate D7 entity translation revision translations
mikelutz	#2746541: Migrate D6 and D7 node revision translations to D8
quietone	Two people did testing. One used my quick instructions which not complete enough so no results. And the other is new to migrations and is working on it when they can.
webchick	Oh that's great!
webchick	Anything we can do to help? Word spreading, or docs reviewing or?
quietone	Just keep asking. Let them know that they don't have to test, unless they want to. Of course, the problem is also the sensitive nature of their data. But then we don't need all the tables, just node* and i18n* and locales_target (off the top of my head).
webchick	Oh great! Is that spelled out anywhere? I could try re-pinging my contacts at work with THAT information and see if it helps (edited)
quietone	I've updated the IS of the Meta with that information. Please improve it. Maybe I should say to contact me? #2208401: [META] Remaining multilingual migration paths
quietone	@webchick ^^

3️⃣ getHighestId() should be able to use any integer destination id

mikelutz

#3091004: getHighestId() should be able to use any integer destination id

4️⃣ Skip_on_empty doesn’t work after a failed migration lookup with multiple destination ids

mikelutz	#3095270: Sql Migration map returns nonsensical destination ids when looking up against a source id row that failed or was ignored.
Nick Wilde (he/him) - DC 2020 DevOps Session Team	@quietone got a q...After apply that patch,id: d6_user_profile_field_instance_translationprocess: langcode: language entity_type: 'constants/entity_type' bundle: 'constants/bundle' field_name: - plugin: migration_lookup migration: user_profile_field source: fid - plugin: skip_on_empty method: row - plugin: extract index: - 1 - plugin: skip_on_empty method: rowBut a different file is patch is done like so:--- a/core/modules/field/migrations/d6_field_formatter_settings.yml+++ b/core/modules/field/migrations/d6_field_formatter_settings.yml@@ -21,12 +21,12 @@ process: source: - field_name -- plugin: skip_on_empty- method: row- - plugin: extract index: - 1+ -+ plugin: skip_on_empty+ method: row(edited)
Nick Wilde (he/him) - DC 2020 DevOps Session Team	Can't the first one also have the first skip_on_empty instance removed?
Nick Wilde (he/him) - DC 2020 DevOps Session Team	Same for: /core/modules/user/migrations/user_profile_entity_display.yml
quietone	Good question. I think they all needs the skip before the extract otherwise Extract could throw a MigrateExecption.
quietone	I will have to review that patch
quietone	I've updated the issue and assigned to myself
Nick Wilde (he/him) - DC 2020 DevOps Session Team	:thumbsup:

5️⃣ Create a way to declare a core migration yml file as deprecated

mikelutz	#3039240: Create a way to declare a plugin as deprecated
quietone	do we need to ping someone to get an opinion on the two approaches?
heddn	I've pinged folks 3 times. I give up :shrug:
quietone	Sigh. I suppose I could try
webchick	Maybe we can also raise in the D9 weekly meeting? AFAIK it holds up us removing deprecated code from D9, so is relevant there as well. Then people get pinged from multiple teams :stuck_out_tongue:
quietone	@webchick thx

7️⃣ Migration dependencies

mikelutz	#3096951: d7_node migration should have dependency on d7_node_title_label migration
mikelutz	#3097314: d7_comment migration should have dependency on d7_comment_entity_display, same for d7_custom_block + block_content_entity_display
mikelutz	I’m still opposed to these, I’ve commented on the second one now. I’ll leave them open a little longer but will likely close them as WaD or WF in another week or so.

8️⃣ Use generators by default in source plugins

mikelutz	#3017237: Use generators by default in source plugins
heddn	@mikelutz @phenaproxima (he/him) thoughts on BC-ness of generators?
mikelutz	We would have to deprecate the rewind function of all the iterators, and then switch to generators in the next Major.
heddn	Deprecate in 9 for removal in 10? Seems like lots of work for low value.
mikelutz	Not really work, just time.
quietone	I'd like to know phenaproxima's opinion
phenaproxima (he/him)	So the main reason why this is a BC challenge is because code that consumes Migrate does so stupidly
phenaproxima (he/him)	It directly calls methods of \Iterator, which is not what one is supposed to do. You’re supposed to traverse over an iterator with foreach
phenaproxima (he/him)	If all calling code did that, we could easily switch to generators with no BC implications.
phenaproxima (he/him)	Now, as for how to resolve this conundrum…not sure
phenaproxima (he/him)	The thing is…it should not be necessary to rewind source plugins. They have had a very ill-defined scope that cuts right to the heart of what’s wrong with the migration API
phenaproxima (he/him)	Their purpose is simple: traverse over a set of records. Period.
phenaproxima (he/him)	The reason they use iterators because, in Ye Olden Days, that’s all we had.
phenaproxima (he/him)	And the presence of those iterator methods has opened them up to abuse
heddn	@phenaproxima (he/him) the BC issue I ran into was when someone wants show a progress bar, you have to figure out the count. Once you figure out the count, you need to rewind the iterator. but that doesn't work w/ generators :disappointed:
phenaproxima (he/him)	I think that rewinding the iterator to get a count, is a misapplication of rewinding.
phenaproxima (he/him)	But, you can use iterator_count($this->getGenerator()) , I think
phenaproxima (he/him)	Or if $this is IteratorAggregate and getIterator() is a generator, then iterator_count($this) works too
phenaproxima (he/him)	One possible approach is a new interface: MigrationSource extends \Traversable. Then MigrateSourceInterface extends MigrationSource, \Iterator. That means source plugins can also be generators if they want to be: MyPlugin implements MigrationSource, \IteratorAggregate
phenaproxima (he/him)	So MigrateSourceInterface continues to be \Iterator, specifically, but calling code that doesn’t need to call \Iterator-specific methods can just accept MigrationSource
phenaproxima (he/him)	That gives us a way to transition core sources to generators without BC breaking
phenaproxima (he/him)	Maybe
phenaproxima (he/him)	My overall point is that migration sources should be traversable and countable. But they shouldn’t necessarily be \Iterator
phenaproxima (he/him)	I think that covariance in PHP 7.3/4 would allow us to transition to this. But that definitely means D9.
alison	(1) "Not really work, just time." lol(2) What's BC?
phenaproxima (he/him)	Backwards compatibility
mikelutz	Or Breaking Change. BC is an autoantonym, and it confuses me a lot.. :stuck_out_tongue:
mikelutz	I’ve added code for BC reasons to avoid having BCs many times, lol.
alison	Ahhhhhhhh ok, thanks!

9️⃣ migrate_drupal’s Variable source plugin always returns a row for processing, even if none of the variables for a migration are set on the source site

mikelutz	#3096969: migrate_drupal's Variable source plugin always returns a row for processing, even if none of the variables for a migration are set on the source site
quietone	This is quite straightforward and I like that it helps to make visible that the Variable source plugin is different.
mikelutz	RTBCed
quietone	:smile:
webchick	This looks like something I could theoretically commit, but I'm a bit unsure what is happening here.
webchick	And also how the tests prove it
mikelutz	The variable source retrieves a row of variables from the d7 database. It always returns one row with entries for each variable requested, whether the variables exist or not. Previously the count function was returning 0 if none of the requested variables existed, which is wrong. The patch fixes that. The test tests it because in the processing of all the added data sets, the return value of count is automatically tested to match the count of the expected data row, which is one. The test would fail without the fix.
webchick	So it's wrong to say a variable doesn't exist when it doesn't..?
mikelutz	The returned row does say the variable doesn’t exist. The problem is that we were falsely claiming no row was returned at all.
webchick	Ok I got turned around by "Previously the count function was returning 0 if none of the requested variables existed, which is wrong."So... even if there are no values for the variables... we still return the ID of the variables regardless... and therefore the count should always be 1?
webchick	That still seems weird/silly to me but
webchick	Like this seems kind of like that other issue about "how come you're importing title overrides for titles I have not overridden" but maybe I misunderstand
webchick	In fact probably. :wink: Anyway, I'm sorry but I have to go pick up my kiddo, but I'll read the backscroll when I get back
mikelutz	The fact that a variable is unset is still valuable information worthy of being processed.
webchick	Right, ok, so it's not "doesn't exist" but rather "exists, but isn't set to anything"?
mikelutz	It’s more like the difference between looping through an array with an unset array item versus an array item set to null. While both basically mean the same thing, in one case you can act on the null and the other you can’t. Even if the variable doesn’t exist, the migration source still needs to send a row saying that so we can process the fact that it doesn’t exist.
mikelutz	And it does send that row, and it always has sent that row. The bug was if you asked that source how many rows it was going to send, it would tell you zero if the variable didn’t exist, even though it was still going to send one.

🔟 Allow logging for non-strings values.

mikelutz

#3047328: Allow logging for non-strings values

1️⃣ 1️⃣ Improve the DX for migrating content entities: view modes, fields, formatters, widgets etc should be migrated per entity type + bundle

mikelutz	#3097336: Improve the DX for migrating content entities: view modes, fields, formatters, widgets etc should be migrated per entity type + bundle
damienmckenna	This looks like it'll be a nice DX improvement.
damienmckenna	Should issues like this have a change notice to let people know about it?
heddn	@damienmckenna we're discussing if it should move to contrib instead of in core. any considerations from your PoV?
damienmckenna	is there an explanation of the rationale to move it to contrib?
damienmckenna	It looks like the work is already done, maybe just needs extra test coverage?
damienmckenna	The point there being, it'd improve core's DX working with migrations, why not do it in core?
mikelutz	Because it’s not an improvement to the core migrations, it’s only useful in contribland.
mikelutz	It provides no benefit to the core migration path.
mikelutz	I’m personally opposed to trying to do it in core, but I think it makes sense as an option for migrate_upgrade.
quietone	Yes, move to migrate_upgrade.
alison	Could someone give a quick overview of differences/"when to use" basics of the main contribs? I'm thinking of migrate_upgrade, migrate_tools, migrate_plus; I'm not sure if I'm forgetting anything.OR is this already on a doc page that I'm forgetting/missing?

1️⃣ 2️⃣ system_maintenance migrations uses incorrect maintenance mode variable in Drupal 7 migrations (edited)

mikelutz

#3096676: system_maintenance migrations uses incorrect maintenance mode variable in Drupal 7 migrations

1️⃣ 3️⃣ Cannot update some file properties during migrate update

mikelutz	#2822545: Cannot update some file properties during migrate update
mikelutz	Commented #2822545: Cannot update some file properties during migrate update#comment-13398551