| welly |
I'd be interested in contributing. I've been working with migrations significantly for quite a few months (at least) now. There's a lot I still don't know, but have some good experience with it (we're migrating about 600 sites) so would be happy to get involved in documentation, code reviews, testing.. |
| benjifisher |
Thanks for joining! It is always helpful to have input from people with "fresh eyes". For example, one of my coworkers recently added this issue, which is not something I would have run into: #3414433: Communicate when there is a missing plugin declaration rather than crashing |
| welly |
One issue I've come across recently is occasionally when you have a required dependency of another migration, sometimes the migration will fail saying that the dependency doesn't exist and I've meant to spend a little bit of time on that to see if it's an issue in the config or something else.
But that one above looks like an interesting piece of work. I might do a bit of investigation into that one. It'll get me to look at the migration api code a bit closer |
| benjifisher |
You can have a look at the NR queue: those are the issues that need code review and testing. https://www.drupal.org/project/issues/drupal?status=8&component=migratio... |
| welly |
Perfect, I've got some time tomorrow to do some contribution work so I'll have a look at that list! cheers! |
| benjifisher |
... occasionally when you have a required dependency of another migration, sometimes the migration will fail saying that the dependency doesn't exist ...
If you can come up with steps to reproduce, then we can do something about it. Maybe, like the issue my coworker added, "something" might mean more helpful error messages. |
| welly |
Yep! I shall certainly do that. I'll try and reproduce it (it's been on and off consistent) |
| mikelutz (he/him) |
Well, required migrations not only have to exist, but they have to run completely with no rows skipped, and it doesn’t really provide a separate error message between the two situations, so my guess is that if you run a drush ms on the required migration you will find unprocessed rows that the migration intentionally skipped. |
| welly |
@mikelutz (he/him) ok that makes sense and perhaps that might be what was happening. Interesting, I'll look into that! |
| benjifisher |
#3247718: Allow process plugins to flag a row to be skipped |
| benjifisher |
I left some comments on the MR a few days ago. Checking now, I see that @mikelutz (he/him) replied there. (I get notifications for updates to issues, but no9t MRs.) |
| mikelutz (he/him) |
I’ve been working on a refactor of MigrateExecutable::processPipeline. |
| mikelutz (he/him) |
I think I have something I like, but it added some new protected methods I’m still writing tests for. |
| benjifisher |
:thankful: |
| mikelutz (he/him) |
I should push up the work without the tests just to see if it breaks any other tests though. I can do that now. |
| benjifisher |
I guess the idea is to add the new properties and methods to the Row class. Then find any place where MigrateSkipRowException is caught, and test getSkip(). |
| benjifisher |
I think that is enough understanding to continue my review. |
| benjifisher |
I think there is a pipeline to test just the test changes in a MR, but not the opposite. |
| mikelutz (he/him) |
pushing up the refactor of MigrateExecutable to see if it breaks the test suite. It still needs tests. |
| mikelutz (he/him) |
A pipeline to test the code changes against the test suite? That’s what the regular pipeline does. |
| benjifisher |
Related: #3365895: When sub_process encounters a row skip, it should skip its internal row, and not bubble up to the outer row is RTBC. |
| mikelutz (he/him) |
Yeah, I’ll refactor this or the other, whichever gets committed first. |
| mikelutz (he/him) |
It will only be a 1-2 line change either way |
| benjifisher |
Right, that is not the hard part. The hard part is remembering to do it. That is why I added an item to the "Remaining tasks" list. |
| benjifisher |
Also related: #3245997: Allow process plugins to stop further processing on a pipeline was Fixed recently. |
| benjifisher |
I am not familiar with that issue. Will we have to make any changes based on that? |
| mikelutz (he/him) |
Well, whichever of 3365895 or 3247718 gets committed first will cause a conflict in the other. They change the same lines of SubProcess, so we won’t forget. |
| mikelutz (he/him) |
And no, I’ve merged the changes from 3245997 into 3247718 since it was committed. Getting that issue committed is what prompted me to start working on 3247718 again. |
| mikelutz (he/him) |
Wow, that refactor actually didn’t break any tests.. That’s a great sign. |
| mikelutz (he/him) |
It’s much cleaner, I just need to finish my unit tests for the two new protected methods. |
| mikelutz (he/him) |
Anyway, still needs work. I think I can clean it up even more. |
| benjifisher |
w00t! |
| mikelutz (he/him) |
But I have more confidence now. |
| benjifisher |
#3398887: Field storage migration fails on re-import |
| benjifisher |
This is NR and tagged for subsystem maintainer review. I am not familiar with it. |
| benjifisher |
(Check to see if I have previously commented on the issue ... no.) |
| mikelutz (he/him) |
Dang… I thought I commented on that one. :disappointed: |
| mikelutz (he/him) |
I guarantee I had one of my long-winded explanations typed up in a tab somewhere and never hit submit… |
| mikelutz (he/him) |
short answer is that it is a bug, needs to be fixed, but there’s some intricacies that make me wonder if this is the right way to do it or not. |
| benjifisher |
Not a quickie, then. |
| mikelutz (he/him) |
Well, it’s NW for tests regardless, but my explaination would have been enough to take the subsystem maintainer review tag off. |
| mikelutz (he/him) |
I’m trying to re-type it up. |
| quietone |
some intricacies
Yea, it is in my too hard basket. :disappointed: |
| mikelutz (he/him) |
Lol, Forgot to hit save again… |
| mikelutz (he/him) |
At least the tab is still open this time. |
| benjifisher |
Save! Save! Save! |
Comments
Comment #2
benjifisherComment #7
benjifisherComment #8
benjifisherIn threads (2) and (9) I meant to reference #3323990: Migrate Drupal reports wrong version of Drupal if pointed at a Drupal 9 or 10 database.
Comment #9
smustgrave commentedWent into #migration and reviewed threads from 1/18/2024. All conversation from that time was captured. There are additional comments but they were added about a day ago, so not part of the meeting.