[meeting] Migrate Meeting 2021-07-22 [#3224655]

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

2021-10-14 - 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.

Agenda items

Add items here

#3220155: Add extra orderBy to d7_file source plugin to avoid random results

0️⃣ Who is here today?

Matroskeen	:wave: Ivan, UA
benjifisher	:wave: Benji: one of the migration maintainers. Massachussetts, USA.
benjifisher	Usually I add an "icebreaker" question to this thread, but I have not thought of one yet.
thhafner	:wave: Thomas, Chicago
benjifisher	Hi, @thhafner! Is this your first time at one of these meetings?
thhafner	yes
benjifisher	We are glad to have you here! Is there something you want to bring up?
thhafner	Ran into an interesting issue and would love to get the migration teams opinion. #3224907: Orphaned state keys after migrate_drupal uninstall
thhafner	Would also love to start contributing back to the Migrate team
benjifisher	Why not comment on 4️⃣ to talk about that?
mikelutz (he/him)	Hello all.
quietone	Vicki

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: 1 (not counting the issue for the meeting).
benjifisher	RTBC: 1
benjifisher	NR: 32, including 2 Major and 7 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️⃣ How can I help? 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, ...

5️⃣ Previous minutes.

benjifisher	Is there anything to follow up? Anything need to be changes in those minutes?
benjifisher	Last week was the quarterly Zoom meeting, so we actually have a recording instead of minutes.
benjifisher	For some reason, I cannot assign credit on the issue. Can one of the other maintainers try? From memory, the following attended the meeting: @mikelutz, @heddn, @quietone, @benjifisher, @marvil07, @dinarcon, @srjosh.
quietone	Added credit to #3223507: [meeting] Migrate Meeting 2021-07-15 and marked as fixed.

6️⃣ Add extra orderBy to d7_file source plugin to avoid random results

benjifisher	#3220155: Add extra orderBy to d7_file source plugin to avoid random results
benjifisher	@Matroskeen, you and I seem to be the only ones here today. Is there more to discuss about this issue? I think you already watched the recording of lastr week's meeting.
Matroskeen	First of all, I suggested more generic approach in the last commits/comments - ordering by source IDs fields.I would like someone to validate before fixing failing tests._Secondly, it was pointed out that adding new order can affect the highwater marks. But what I'm seeing - it's not quite correct for current source plugins that already provides orderBy In this case, order by highwater is coming 2nd, when the main query is already defined.I mentioned this in the last comment. (edited)
benjifisher	I have not had a chance yet to look at the implementation, so I am just replying to your comments.IMO, the extra ORDER BY clause should come after any other sorting. That is the intention of the issue.I have not looked at how you implement it, but if it works as you say then :+1:.As long as it works that way, it should not break highwater marks, nor anything else.I have never looked at the code for highwater marks, but I have seen comments from others that it is buggy. If the ORDER BY clause added for the HW mark comes after other sorts, then that might explain some of the bugs. Have you looked for existing issues related to this?
benjifisher	@mikelutz (he/him), are we missing anything here?
mikelutz (he/him)	sounds about right. I will say that the implementation of highwater support in SqlBas is designed for simple querys and having an orderby in a derivitive class will break that implementation. It’s important to note though that highwater is all done in the source plugin, there’s nothing in the general migration or executable classes that touch it, so if you have a more complex query but want to make highwater work, it’s certainly possible to override the initializeiterator and/or the highwater methods to account for more complex queries and keep them from breaking.
benjifisher	I had a look at the MR.if the source plugin already contains orderBy by same ID field(s) - nothing changes, the second one will be ignored;Maybe I am reading too much into this claim. If a source plugin extends SqlBase and does the typicalpublic function query() { $query = parent::query(); $query->orderBy(...); return $query;}then it will not work. I thought your comment meant that you had arranged it so that the new orderBy() would come after the one in the child class.
Matroskeen	@benjifisher, if the plugin overrides the query() method, then it becomes primary sorting order (if there is no order in parent class yet), or secondary if the order exists.The order by highwater or source IDs (suggested in my approach) is added later - in initializeIterator() method, when the query is already finalized and modified by source plugin. (edited)
Matroskeen	As for the highwater marks, maybe we should be more aggressive and remove existing orderBys if the highwater is specified, but that's another topic to discuss. (edited)
benjifisher	My mistake: I was looking at the MR in the web UI, and I did not click "Show 20 [more] lines" enough times to see what function was being modified.

7️⃣ Uninstalling migrate_drupal_ui Does Not Remove Stored Migration Credentials From the Database

benjifisher	#3224907: Orphaned state keys after migrate_drupal uninstall
benjifisher	From the description, that looks like a bug to me.
benjifisher	I see some discussion of this problem in this channel yesterday. In addition to what it says on the issue, @Matroskeen said,Try to look at:Drupal\migrate\Plugin\migrate\source\SqlBase::getDatabase()
benjifisher	It should be pretty easy to implement hook_uninstall() and remove the items from the key_value table.
thhafner	I am happy to work towards implementing a fix, just wanted to get the team’s opinion that this was, in fact, a bug.
benjifisher	Mission accomplished. :wink:
benjifisher	We will also want a test for the issue. I think it will need to be a functional test. Maybe we can add a few lines to an existing test:check that the credentials are storeduninstall migrate_drupal_uicheck that the credentials have been removed.The test cannot repeat the other "Steps to reproduce" from the issue description: Drupal core does not provide a way to run migrations from the CLI. You need Drush to do that.
thhafner	Sounds good
dinarcon	I think this is somewhat related with these discussions...
dinarcon	https://drupal.slack.com/archives/C226VLXBP/p1619705689366300?thread_ts=...
dinarcon	https://drupal.slack.com/archives/C226VLXBP/p1586207291165500?thread_ts=...

8️⃣ 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.
Matroskeen	Thanks @benjifisher :raised_hands:

Participants:

benjifisher, quietone, Matroskeen, mikelutz (he/him), thhafner, dinarcon