Problem/Motivation

Over in #2934962-8: Ship local audio and video media types in Standard, @xjm pointed out that the config Media ships in Standard is missing certain things that it should rightfully inherit from Standard's other modules after installation (dependencies, third party settings, etc.)

Proposed resolution

Install Media as part of Standard (not after the fact), then re-export all of Media's configuration. The difference between the shipped config and the exported config is the patch that should be filed in this issue.

Remaining tasks

Do the thing I just said. Then commit the patch.

User interface changes

None.

API changes

None.

Data model changes

Nil.

CommentFileSizeAuthor
#8 interdiff-2934991-2-8.txt384 byteschr.fritsch
#8 2934991-8.patch4.48 KBchr.fritsch
#2 2934991-2.patch3.9 KBstarshaped

Comments

phenaproxima created an issue. See original summary.

starshaped’s picture

starshaped
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2934991-2.patch3.9 KB

Exported media configuration and created a patch with the updates, removing uuid and _core from each file.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

This looks great; RTBC from me.

However @xjm and I briefly discussed this snippet:

+++ b/core/profiles/standard/config/optional/core.entity_view_display.media.image.default.yml
@@ -3,7 +3,6 @@ status: true

@@ -3,7 +3,6 @@ status: true
 dependencies:
   config:
     - field.field.media.image.field_media_image
-    - image.style.medium
     - media.type.image
   module:
     - image

We believe that this may be related to the problem we turned up over in #2923664: Missing image styles during "Media" install causes issues in media form and view display.. It bears further discussion to figure out what's going on here, and what, if anything, we need to do about it here. Therefore, I'm leaving this at the review stage.

Status: Needs review » Needs work

The last submitted patch, 2: 2934991-2.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Re #3: I think that is just a leftover from #2934850: Media Images should be rendered at a reasonable size by default which was recently committed.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented

I think we shouldn't add the path fields to the config because if we do so, we have to add the path module to the list of media dependencies.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented

They are already part of core.entity_form_display.media.audio.default and core.entity_form_display.media.video.default. So if you install in the standard profile, uninstalls path and then will install media, you will not get these configs. So not 100% sure whats best to do.

If we want to ship with the path config, we probably have to add path to the list of modules in MediaJavascriptTestBase to fix the tests.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2934991-8.patch4.48 KB
new interdiff-2934991-2-8.txt384 bytes

Thought about a bit more about that. Since all the configs are optional, it will not really break if path module is not installed. The config will just not be imported.

So to fix the tests, I added path module to MediaDisplayTest

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Needs review » Needs work

I agree with #5 that the medium image style dependency removal is expected, it should probably have been removed in #2934850: Media Images should be rendered at a reasonable size by default but we missed that.

Just tested the "missing path module" scenario, and unfortunately things don't appear to work as expected. Nothing breaks, but if the core.entity_form_display.media.*.default.yml are not installed, the source fields disappear from the form. Couldn't dig too much into the root cause, but in any case we want to make sure that scenario works, or at least not include anything path-related, as suggested in #6

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented

This is fixed with Tests in #2940205: Some media default configuration does not install as expected

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Status: Needs work » Closed (duplicate)

Closing this in favor of #2940205: Some media default configuration does not install as expected (since that has more tests). Thanks everyone, added issue credits to the other issue as well.