View mode doesn't display in dropdown if numerical

Fun bug! I've dug in a little bit and the source of the problem appears to be the following line in FilterPluginCollection::initializePlugin():

      $configuration = NestedArray::mergeDeep($configuration, $this->configurations[$instance_id]);

Before that, the important part of the media_embed settings looks like this:

[
  'allowed_view_modes' => [
    'default' => 'default'
    450 => 450
  ]
]

But afterwards the numerical array key gets ruined. It then looks something like this:

[
  'allowed_view_modes' => [
    'default' => 'default'
    0 => 450
  ]
]

So later when array_intersect_key is called in EditorMediaDialog to get the available view modes we get unwanted behavior.

I've gone ahead and made a one-line change that fixes this bug for me locally. I'm wondering if this will cause anything else to fail. Setting to NR to trigger tests on the patch, but this issue will still need a fail test.

I'm also adding a related issue and tentatively changing the system to which this bug applies.

Ok, back to the fail test. Here's a fail test that updates an existing test such that one of the view modes has a numeric id. The way this will fail is that the View Mode select element won't be found because the form thinks there's only one available View Mode.

I also changed a couple assertSame calls to assertEqualsCanonicalizing so that the order of array elements doesn't matter.

Alternatively, I could add a fourth view mode to this test case instead of replacing one of the existing view modes if the community would prefer that.

Ignore that random fail in LayoutBuilderDisableInteractionsTest, a well known random fail.

The relevant test failed as expected:

1) Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testViewMode
Behat\Mink\Exception\ElementNotFoundException: Select with id|name|label|value "attributes[data-view-mode]" not found.

/var/www/html/core/tests/Drupal/Tests/WebAssert.php:228
/var/www/html/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php:1236
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

and was fixed by the other patch.

Fix looks right for the job, code is clean and test coverage is there.

I'm uneasy about altering the existing view mode in the test to include the numerical key rather than creating a third one, but I cannot give any kind of reason as to why which may just mean it comes down to personal preference.

So, since that's the only piece of feedback I would have, this passes community review as far as I'm concerned.

Thanks for the review, @darvanin.

I'm uneasy about altering the existing view mode in the test to include the numerical key rather than creating a third one, but I cannot give any kind of reason as to why which may just mean it comes down to personal preference.

My thinking there was that I was able to avoid adding lines to the test. The downside of doing this was that the message in the fail test was not very clear, which is why I felt the need to give some extra info in #4.

I'm happy to change the test coverage though. Maybe we can see what the core committer thinks?

Regardless, I'm going to change the branch to 9.4.x. Hopefully it doesn't need a re-roll. I think I'll have to manually trigger the tests initially.

The fail test is re-running automatically. I'm re-uploading the good patch here so that's the one that tests run on.

Back to RTBC per #7 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

Committed and pushed f3b24d25a0a to 10.0.x and 2e7b6ca227f to 9.4.x and adb54155184 to 9.3.x. Thanks!