Move File/Image media type into Standard once Media is stable

Follow-ups shouldn't use the original issue as parent. It can't be a sub-task of closed issue. Not sure if there's a diferent parent we should use, maybe the ideas issue?

Actually, I would recommend to keep it/move it now to the install profile.

I think having the configuration in the install profile is better because having it in the module will interfere with existing install profiles like thunder, lightning and so on as they already have their own media types they provide, it's not easily possible for an install profile to prevent configuration from a module to be installed (all you can do is delete it in hook_install()). Pretty sure that they will want to keep their own media bundles/types as existing data would otherwise be very hard to convert.

As I explained in #2831936-169: Add "File" MediaSource plugin, but would like to reiterate here:

As of that patch, several Media tests are deleting the File media type in their setUp() method. This is because the default configuration is, at the time of this writing, packaged with Media module rather than an install profile. Once we move the configuration into Standard, we will be able to delete those lines in the tests' setUp() method, since the tests will then be running without any default configuration (as they were originally written to do).

This is blocked on Media becoming stable.

We need to move the image config back as well. #2831937-70: Add "Image" MediaSource plugin

#8++

Media is stable now, so we are able to un-postpone this, right?

Last week i tried to run Thunder with core media and it turned out, that we are having some problems when the config lives in the media module and not in the profile. We are providing our own media type image with some third_party_settings and using a different field name for the source field.

So things would be much easier, if we could move the config into the profile.

As discussed with @xjm during media meeting, we will unpostpone this to discuss if we could do this. As @berdir mentioned in #3 having the config in the module, brings some problems for install profiles.

I would propose to move the entity displays, field config and media types into config/optional of the standard profile. In that way it will only be installed if someone enables media module. Same as current behavior. But it would make it much easier for distributions to ship their own config.

Here is a patch to my suggestions from #12. I also moved the field storage, because they are coupled so tight to the media types

We'll also need to revert the test changes from the other issue that originally moved it I guess.

But I'm very +1 to this, this would make it hard for existing install profiles to migrate from media to media_entity (and keep the media types the same for new and existing types).

We can not prevent config from being installed, best we could do is deleting them again in hook_install().

It also needs to happen before 8.4.0, afterwards it might be a BC break?

All the tests should be fixed

+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaViewsWizardTest.php
@@ -14,6 +14,21 @@
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = [
+    'system',
+    'node',
+    'field_ui',
+    'views_ui',
+    'media',
+    'media_test_source',
+    'media_test_type',
+  ];

it's not necessary to duplicate the module list from the parent, just add the additional ones. Not sure I understand why media_test_type is required now for this test? Is it because we need a media type to exist? Why in some cases we create one through the API and in this case we rely on the test module?

Ok, i changed it to use the API for a media type creation. We are relying just once on the media_test_type now. When we are testing the creation from default configuration.

Thanks, looks good to me as I mentioned, this would be quite important for install profiles to get in before 8.4.0.

+++ b/core/modules/media/tests/src/Functional/MediaFunctionalTestCreateMediaTypeTrait.php
@@ -61,6 +62,11 @@ protected function createMediaType(array $values = [], $source = 'test') {
+      ->setComponent($source_field->getName(), ['type' => 'file_generic'])

This doesn't work for an image source. The type must be fetched from the source field before somehow

I just removed the type, so the WidgetPluginManager will handle it.

What exactly was the problem and why didn't tests fail?

We might get some test fails due to #2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types, re-testing..

Regarding #22: createMediaType() can be used to create media types from different sources. Having 'type' => 'file_generic' would only work, if the source field is a file field.

I changed the field name from field_media_file_1 to field_media_file in the new tests of #2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types

Yay, this patch is making the test coverage added by #2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types more elegant — it addresses @effulgentsia's feedback at #2835767-70: Media + REST: comprehensive test coverage for Media + MediaType entity types and #2835767-73: Media + REST: comprehensive test coverage for Media + MediaType entity types.

Which means that for this patch to pass tests, it needs to make tiny modifications to the test coverage introduced by #2835767.

#25 and #26 are cross-posts. The solution in #25 is incomplete.

In this patch, I'm also importing the other changes #25 made.

I think this would still be OK to commit during rc, but otherwise would need to wait until 8.5.x.

The only way I can see this being an actual bc break is if someone's relying on the default configuration existing, then it doesn't, but we don't offer any guarantee that default config exists. Is there any other implication I missed?

I think you're right, @catch.

Also, from #drupal-media on IRC:

16:35:18 <chrfritsch> Ok, here are my updates: I worked on https://www.drupal.org/node/2883813 the last days, because it's essential for distributions to use media. So i think it's fine now and could be pushed back to RTBC now
16:35:19 <drupalbot> https://www.drupal.org/node/2883813 => Move File/Image media type into Standard once Media is stable #2883813: Move File/Image media type into Standard once Media is stable => 29 comments, 5 IRC mentions
16:36:26 <chrfritsch> So would be really nice to get that landed in 8.4
…
GaborHojtsy> chrfritsch: hm, not sure https://www.drupal.org/node/2883813 is feasible for 8.4, the RC is about to be released after all
16:43:47 <drupalbot> https://www.drupal.org/node/2883813 => Move File/Image media type into Standard once Media is stable #2883813: Move File/Image media type into Standard once Media is stable => 29 comments, 6 IRC mentions
16:44:04 — GaborHojtsy not a release manager though :D
16:47:29 <chrfritsch> GaborHojtsy: Berdir thinks this could be a BC break, in that case it should be fixed before 8.4
…

16:48:41 <chrfritsch> xjm: What's your thoughts on that?
16:48:56 <xjm> chrfritsch: Did you link the correct issue? 8.4.x is totally set now and in RC
16:49:18 <xjm> chrfritsch: Looking more closely
16:49:57 <xjm> Hmm
16:52:21 <xjm> chrfritsch: So this just adds optional config to Standard, right, and doesn't actually enable the module in Standard?
16:52:32 <chrfritsch> xjm: Correct
16:53:05 <chrfritsch> xjm: It just moves the config from media module in standard config/optional
16:59:46 <xjm> chrfritsch: catch is asking how it's actually a BC break
17:00:03 <xjm> chrfritsch: He commented on the issue
17:00:19 <xjm> Per our release policy nothing more should be going into 8.4.x for like a month now
17:00:29 <xjm> but we can discuss more on the issue I guess
17:01:33 <chrfritsch> xjm: Ok, thank you

Slam-dunk.

Yay, I am so happy to see this go in. Very excited for where this leads media in 8.5 :)