Decouple text_with_summary from tests but not migrations

I left one question on the MR, but I didn't do a complete review because I realized how late it is and how tired I am. So I'm leaving the status at NR.

I left some comments on the MR.

I noticed some assertions that were migrated to Text module stats that don't seem to be necessary. For instance, TextWithSummaryMigrateFieldFormatterSettingsTest seems to have a lot. Only 5 of the assertions were removed from the original MigrateFieldFormatterSettingsTest. I couldn't figure out if these assertions that aren't related to text-with-summary fields are necessary or not. Was their inclusion intentional?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I was kind of confused by the removal of the text-with-summary fields from the D7 fixture. After all, it's not like the field is being removed from D7. But I guess you have to do that because we wouldn't want to change the migration plugins to migrate D7 text_with_summary fields to text_long, right?

I double-checked the MR. I can't find anything else to give feedback about. So I'm going to RTBC it.

Left some questions on the MR

I'm not sure we can change the D7 fixtures like this. They represent what the field type was then. We can't go back in time and change that.

I haven't actually reviewed the MR yet, but I think we can rewrite history because text long also existed then. So we're removing coverage of text_with_summary from the core tests (which would eventually live in the text_with_summary contrib module) but not the ability to migrate from text_with_summary to text_with_summary which will require installing the contrib module.

I also do not think that existing fields in the migrate drupal text fixture should be changed. All the fields were added for a reason and test a particular possibly Drupal 7 configuration. By changing them we risk losing test coverage for a configuration that can exist in the wild.

Any needed new fields should be added. And they should be added using a Drupal 7 site so that we are sure all the configuration is as D7 site would create and that the fixture works in a D7 site.

Also, I still have my D6/D7 setups available. I can create the needed fields and update the fixture. I just need the details for the news fields. And I do wonder how they are different from the existing 4 text_long fields in the current fixture.

Wait a minute. Now that I have had time to eat, I realize there is something wrong here. We only need to change migrations and migration tests if we are changing what the text_with_summary field migrates to in D11. I gather the plan is to deprecate text_with_summary in D11 and that is also when Migrate Drupal will be deprecated. So, both will be deprecated and migrate in core is not responsible for the migration to the new the destination, it will handled by a contrib module.

Checking the parent issue, that is what berdir and catch agreed to, See #3427095-18: [Meta] Deprecate text_with_summary and #3427095-19: [Meta] Deprecate text_with_summary. So, the migration and migration tests should remain unchanged.

Updating the title accordingly.

@smustgrave asked for feedback on the approach here.

I think the updated scope makes sense (it seems like the new test could move cleanly to contrib with Migrate Drupal without altering the coverage or the history of the migration under test).

That said, honestly I'd appreciate feedback from a Migrate maintainer to confirm that the updated decoupling here matches the goal.

Thanks!

When I changed the title I expected that all the migration work would move to a separate issue. Migrate is a 'special snowflake' and best reviewed separately. This is the way it is done for removing modules that are going to contrib. This just has an intermediate step of moving to a another core module first.

@smustgrave, apologies for any misunderstanding on this point.

Reviewed the MR and it looks good to me, the changes are now cleanly scoped to use a different field type when we're not specifically testing text_with_summary..

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!