@heddn

Thanks for your kind fixes. Now, why didn't tests catch that?

is the thought to merge that over here?

Also, merge conflicts still seem to exist.

Posted feedback on the MR.

I assume that once this gets merged, we'll add the link target to https://www.drupal.org/node/3223395#s-migrate-drupal? Ah, just reviewed the IS. It is in there as next steps.

I don't see a CR though. Do we still need to add that? Otherwise, +1 on RTBC.

Needs a rebase.

Can we add some screenshots showing before/after the problem and its fix?

Can we get before/after screenshots showing the problem and fix?

Lets land this. Its very disruptive. But it will always be disruptive. And much of the RTBC queue is cleaned out right now, so this is about as good as it gets.

Thanks for your contributions.

MR 102 looks like an easier solution. But I'm not sure which is the better approach. I don't use json parser all that often myself to have an opinion. Any suggestions? MR 102 doesn't need a rebase/reroll but 103 does (if the later is the preferred solution).

heddn → made their first commit to this issue’s fork.

Reasonable plugin. Needs tests though. And there's some feedback posted on MR.

Thanks for the contributions.

There's no code to review. Back to active.

I'm not sure how the actual fixes in MR 96 are fixing the problem. Is there any way to add a test for this?

NW for adding the final bits.

The approach in MR 111 seems reasonable.

Is there any real world testing for MR 109? What's in there looks reasonable.

I think I'd like to see #3227245: Entity lookup doesn't work with multiligual content → land first to make the work here cleaner and easier to review.

I changed the base branch and attempt to rebase using the gitlab UI. There are still conflicts though. In general, the changes here all make sense. Great work here.

I'll take comment #7 as a good way to keep things a bit more DRY. If you still feel strongly about the addition of this feature, feel free to re-open and propose some alternatives.

can't those values be empty arrays? that's what I'd expect given the return type of getMigrationDependencies and getMigrationTags. They should always return an array. With that in mind, I'm going to close this as won't fix. If you strongly disagree, feel free to re-open and provide some examples of where an empty array might not be possible.

heddn → made their first commit to this issue’s fork.

Thanks for the work here.

Thanks for your contributions.

Thanks for the work on this everyone.

Nice idea here. Added some comments on the MR.

There's no adjustments to tests and the existing MR needs a rebase.

Node body addition got removed in core at some point in the recent-ish past. We'll need to add it to all our config exports.

I like the idea here. Let's roll this into an MR and see how tests like it.

Tests are failing.

There are still some eslint failures.

Thanks for the fixes.

Thanks for the contributions.

heddn → made their first commit to this issue’s fork.

The latest patch should be rolled into an MR.

this needs to be rolled into an MR. Also, I think the last patch is missing some bits as part of the patch.

The patch should get added to a MR.

This is postponed on ✨ Add more metadata to field type properties to identify text with a format and separate user provided text from machine names Active landing first.

Thanks for the conversion to Markdown

heddn → made their first commit to this issue’s fork.

Improving the title of this issue.

Thanks. Added.

This needs a rebase.

Assuming I didn't disqualify myself from marking RTBC from resolving a trivial merge conflict. And assuming this returns back green, LGTM.

I don't think we want to require administer menu to add/remove/update menu links.

Thanks for the contributions here.

This needs some tests.

I like what we're doing here. Can we add test coverage though?

Looks like this was rebased and some features retained. But no IS update. Can we get an update on what this is doing now?

I'm not seeing any phpcs errors any longer. I think they must be resolved in another ticket.

heddn → made their first commit to this issue’s fork.

I think this makes sense. Once this lands, I'll commit 🐛 Error when removing a menu Needs work

heddn → made their first commit to this issue’s fork.

Is this still an issue in the v2/v3 more supported version of this module?

I think this is closed won't fix. If I'm reading things wrongly, feel free to reopen.

I think this is already resolved. At least I can't find it in the code base any longer.

Can we asses if this is still an issue in a more fully supported v2/v3 version of this module? Marking postponed until we can confirm that.

I think this might be a duplicate of ✨ Add support for Linkset functionality being added to core Active

This should be based on top of v2/v3 and added into an MR for tests to execute.

is this still an issue in v3 or v2? Since this is a feature request, I'd like to see it added there first.

Let's mark postponed and see if anyone can provide steps to reproduce.

Given https://www.drupal.org/project/group_content_menu_bundles → as a possibility, I'm going to go out on a limb and suggest it is the solution desired here. If that isn't the case, feel free to re-open and provide some suggestions on how you'd like to see this move forward.

Can we add tests for this? And since v1 is a bit on life support, it would be better to target any changes here first against v3 or v2.

No response in a bit of time. If this is still an issue, feel free to reopen and post an MR with a possible path forward.

Can we add a test for this? And ideally we fix it in 3.x/2.x first since v1 is sadly not getting a lot of attention at the moment.

I think we can't add this unless the other issue lands in core. Can we do some type of exists check to add this in before it lands? Also moving this to v3 so we can backport.

With support for v1 of this module going away, can we test if this is still an issue in version 2 or 3 of this module?

With support for v1 of this module going away, can we test if this is still an issue in version 2 or 3 of this module?

With support for v1 of this module going away, can we test if this is still an issue in version 2 or 3 of this module?

With support for v1 of this module going away, we should target to get this into version 2 or 3 of this module.

With support for v1 of this module going away, can we test if this is still an issue in version 2 or 3 of this module?

With support for v1 of this module going away, can we test if this is still an issue in version 2 or 3 of this module?

With support for v1 of this module going away, can we test if this is still an issue in version 2 or 3 of this module?

I'm not sure this is still a problem. If it is still, feel free to re-open with steps to reproduce in the most 6.1 release.

I'm not sure this is still a problem. If it is still, feel free to re-open with steps to reproduce in the most 6.1 release.

I'm happy to report that both https://www.drupal.org/project/migrate_tools/releases/6.1.3 → and https://www.drupal.org/project/migrate_tools/releases/6.1.x-dev → were just tagged.

But that's way past the 5.x release being requested. so I think this is safe to just close.

I wonder if we can mark this as closed given the passage of time?

I'm happy to report that both https://www.drupal.org/project/migrate_tools/releases/6.1.3 → was just tagged.

I'm happy to report that both https://www.drupal.org/project/migrate_tools/releases/6.1.3 → and https://www.drupal.org/project/migrate_tools/releases/6.1.x-dev → were just tagged.

We no longer have 2 code streams for drush commands. So I think this issue has served its purpose.

I think we have ways to do this in standard drupal config override methods in modern versions of drupal. If this is still a feature request, feel free to re-open.

The fix here should be solved upstream in Drupal core. Not much we can do in this module.

✨ Migrate support for deleting items no longer in the incoming data Fixed is closed as fixed. Is this still an issue?

Can you provide a breaking test that shows the results?

I'm happy to accept MRs that change the current behavior here.

Once you get an idea on cause, feel free to report back. For now, marking as needs more input needed.

I like the idea here. This should help in certain situations. Needs a quick rebase. Ideally we'd add test coverage. I think that is possible.

Is this the only way to make it happen?

For the addition of a new feature, the addition of test coverage would be super great to see. Also feedback in #10 should be responded to.

heddn → made their first commit to this issue’s fork.

I'm not as convinced this is a spelling error. Can you expound on the issue? Also, the patch should be rolled into an MR so we can have tests executed.

This needs a rebase.

The latest work here should be rolled into a MR. Also an update on the IS to shrink it and make more explicit what is the issue and possible solution.