d7_field_formatter_settings should migrate the default view mode to default not full

This definitely looks like a bug to me…

It was introduced in #2416765: Migrate Drupal 7 Field/Instance/View mode settings.

Just checked the D7 fixture and all the content types have this, so, unless I am mistaken, the situation in the IS is not tested.

What does the tag 'migrate-d6-d7' mean here?

I updated the fixture such that a_thirty_two_character_type_name has custom settings for the Full view mode. Then I changed the body to use text_trimmed instead of text_default when in Full view mode.

I chose that node type because it only has one field and therefore has a relatively small effect on the fixture. There would be a much bigger diff if we were to re-configure one of the other node types, which all have many fields.

Here's a patch that makes the fail test pass locally. I'll be interested to see if it breaks anything else.

This mapping from full to default appears to have been intentional. I couldn't figure out what the thinking was in the referenced issue. And I haven't stumbled upon any comments that shed light on the thinking. It makes me wonder if there's something I'm missing...

Lets try these...This has some updates to the fail test. The fix is the same as in #7. I *think* everything will pass now.

Note that the number of entity_view_modes after upgrade increases from 14 to 18 due to the patch. That's because prior to these changes, the default view mode gets migrated as if it's the full view mode. At least I think that's what's going on. Here's the list of view modes for reference:

Before patch:
["aggregator_feed.summary","aggregator_item.summary","block_content.full","comment.full","node.custom","node.full","node.print","node.rss","node.search_index","node.search_result","node.teaser","taxonomy_term.full","user.compact","user.full"]

After patch:
["aggregator_feed.summary","aggregator_item.summary","block_content.full","comment.default","comment.full","node.custom","node.default","node.full","node.print","node.rss","node.search_index","node.search_result","node.teaser","taxonomy_term.default","taxonomy_term.full","user.compact","user.default","user.full"]

Edit:
It looks like we don't actually want the default entity view modes like node.default to exist. I just checked a normal D9 site and you can't export that view mode. So my fix is not good, and most of the changes I added to tests in #10 were off the mark.

The fix is going to be more complicated that I had hoped. Time to take a step back.

Here's a fail test that doesn't go wild changing a bunch of tests that shouldn't be changed. It's a slightly nicer version of the one in #6. The patches in #10 should probably be ignored...

And of course I forgot the patch...

I think I'm on to something now. I've updated the d7_view_modes and d7_field_formatter_settings migrations along with comments to explain what's going on. I think the current bug has two sources.

1. In d7_view_modes the default view_mode gets migrated and re-named as if it's the full view mode.
2. In d7_field_formatter_settings any special settings for the full view mode get migrated as if they are the default settings. (Described in the IS.)

You can check the diff to see how those issues are addressed.

I'm expecting this to cause RollbackViewModesTest and MigrateViewModesTest to fail at this line:

$this->assertEntity('comment.full', 'Full', 'comment');

I'm pretty sure that the only reason the assertion works currently is that the default view mode gets migrated into the full view mode. I should probably get a second opinion before I go in again and start changing tests, but I think we'll need to change that one. I think it's asserting that the code is broken.

I'm going to set this back to "needs review" since the latest patch failed the way I anticipated and I'm looking for feedback.

@danflanagan8, thanks for working on this issue.

I applied that patch and installed the fixture into a D7 site to check the changes. They look good and will allow testing of this problem.
I'd like to know the history of why default is migrated to full and have asked phenaproxima in #migration. I didn't review the patch or anything else.

Comforting to know that others find this a bit confusing, #2844203: Improve/Simplify situation around Default/Full view modes/view displays

Looking at this again.

So, why is the d7 view mode 'default' is mapped to 'full'? No reason is given in #2416765: Migrate Drupal 7 Field/Instance/View mode settings. Am I missing something about D7?

Spent some time going over how view modes work in D7 and D9 and have a better understanding of it now. Thanks to phenaproxima and @xurizaemon for their support.

I then reviewed the patch and ran the tests with my improved understanding. I found that this works perfectly. The default view mode is handled correctly now.

In reviewing the patch I thought the comment in d7_field_formatter_settings could be improved but decided against it because it refers to d7_view_modes.yml which has additional useful information. So, not hard for someone to find and no need to add more detail in d7_field_formatter_settings.

So, this is ready for RTBC but before that I want to look at the corresponding D6 migration.

Typically, fixes are made for Drupal 6 and Drupal 7 in the same patch. I think the same can be done here with out too much work.

Make the same change to d6_field_formatter_settings
Modify d6\MigrateFieldFormatterSettingsTest, so that tests on the 'default' mode are changed to 'full' that should just work. Then add assertions on the default mode, which is the one that has changed.

I would make the changes but I want to retain the right to RTBC.

First order of business, here's an updated patch for the D7-D9 fix where a couple obsolete assertions have been removed. Now that the default view mode is properly migrated, the full view mode is not created for comments and users.

Second order of business is D6...

I dug in a bit, even getting my first ever D6 site running locally! From what I can tell, the default view mode does not exist in D6. It looks to me like the D6 full view mode theoretically could map to either default and full in D9.

I don't see how the D6 to D9 migration could have the same problem described in the IS since it doesn't appear that D6 supports separate configuration fro Default and Full view modes (since default view mode does not exist).

Of course, I'm veeeery shaky with D6 so I could be wrong.

Yes, it makes sense that the full mode doesn't exist on the destination. The changes to the test look correct and include an improvement in testing the rollback of node.custom.

Thanks for investigating D6-land. given that then we need to find out why d6_field_formatter_settings has the same pipeline for view_mode as d7, specifically the static_map. That was added in #2278613: Body text display is blank for migrated content types which was committed before the d7 version.

  view_mode:
... trimmed..
        - 1
    -
      plugin: static_map
      bypass: true
      map:
        full: default

Maybe the formatter setting should be applied to the default and the full view_mode? I don't know and am too tired to think on it. That is all I can do on this tonight.

@quietone, nice find on that related issue! And part of your comment looks very important to me:

which was committed before the d7 version.

This all leads me to make two bold claims:

1. The D6 view mode migration works as designed following the linked issue.
2. The D7 view mode migration mishandles the default and full view modes because we copied off of the D6 pipeline. We're fixing this mistake now.

It looks to me like we're safe to focus on fixing only the D7 view mode migration in this issue.

@danflanagan8, I read your comment yesterday, and poked around a Drupal 6 database, and thought it correct but wanted to sleep on it. Today, with a clear head I do agree with no changing the Drupal 6 migration. Thanks for getting this done!

I already reviewed the patch in #26 and on another look I see no problems with it.

Therefore -- Off we go!

5 tests failed, 4 are know random failures from LayoutBuilder listed on #2829040: [meta] Known intermittent, random, and environment-specific test failures.

• ContentPreviewToggleTest
• InlineBlockPrivateFilesTest
• InlineBlockTest
• LayoutBuilderTest

The other one is ToolbarAdminMenuTest whichi passes tests locally.

That's nuts. I've never seen 4 random fails on the same thing. Kind of makes the random fails seem less random, eh?

That ToolbarAdminMenuTest failure is weird since that's not a FunctionalJavascript. A quick (lazy) search didn't find an issue for it.

I'm going to set back to NR to trigger tests again. and then set the patch to re-test.

All the tests passed when I re-tested. I'm going to set back to RTBC since it only got kicked off of that due to random fails.

Tests on submitted patches keep failing because an outdated schema. The error isn't caused by the patch.

Restoring RTBC, the test failure was fixed in #3255836: Test fails due to Composer 2.2

That was a random failure in Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest

This is a known random failure: #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest)

Setting back to RTBC.

More random fails in LB tests. Back to RTBC.

The Migrate Drupal Module was approved for removal in #3371229: [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3522602: [meta] Tasks to remove Migrate Drupal module and the removal work in #3522602: [meta] Tasks to remove Migrate Drupal module.

Migrate Drupal will not be moved to a contributed project. It will be removed from core after the Drupal 12.x branch is open.