Closed (fixed)
Project:
Drupal core
Version:
8.4.x-dev
Component:
media system
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
28 Sep 2017 at 07:32 UTC
Updated:
30 Nov 2017 at 18:19 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
chr.fritschJust setDisplayConfigurable on true for the display context.
Comment #4
chr.fritschOk, fixed the default config
Comment #5
phenaproximaDiscussed briefly with @alexpott and @chr.fritsch, and we decided this will need some sort of test to ensure that we don't accidentally regress the setting later. Something as simple as asserting that the base field is configurable (i.e., in a kernel test) would be sufficient.
Comment #6
chr.fritschHere is the test
Comment #10
chr.fritschOk, now it should be fixed
Comment #12
seanbLooks good. We might want to remove the empty lines but that could probably be done on commit. Or not..
Comment #13
phenaproximaCleaned it up.
Comment #14
gábor hojtsyHm, how is it shown before the patch? As a base field added to the display?
Comment #15
chr.fritschBefore the patch, you were not able to configure the name field on the 'Manage display' page. That means all the logic, if the name should be displayed or not, were in the media.html.twig file.
Comment #16
gábor hojtsyOk let me do this question differently then :) How is that without changing that twig file, we get it configurable? This is media.html.twig:
Comment #17
gábor hojtsy@chr.fritsch rightfully pointed out that the Media class annotation connects the name media field to the label entity key and that leads to the label variable in twig being populated with name. The patch makes it configurable without needing to modify any templates as a consequence.
One more question from here:
Is this enough as an "upgrade path" for existing sites? We'll definitely not update their active config, so no need to even look into adding the configurable name there, just wondering about whether this makes it possible to add it to the active config on sites that get updated.
Comment #18
chr.fritschYes, it is. The name field is already in the display. We don't change that. We just enable the name field to be configurable. So a cache clear is everything that needs to be done, to see the name field on 'Manage display'.
I rerolled the patch, because #2883813: Move File/Image media type into Standard once Media is stable landed. There is no interdiff, because only thing that changed were the path of the entity_view_display files.
Comment #19
chr.fritschRe-RTBC per #12
Comment #21
gábor hojtsyYay, ok, I got it finally. Superb. Committed.
Comment #22
rwam commentedIt would be great to have a patch for current 8.4.x too, because the provided patch failed on current version 8.4.0. So I've tried to create a patch for the 8.4.x branch.
Please have a look and sorry if I do something wrong.
Ciao
Ralf
Comment #23
gábor hojtsyI'm not 100% sure this is allowed on 8.4.x, would need to check with other committers. However the config should definitely not be in the standard profile in 8.4.x.
Comment #24
rwam commentedHi, I take the config from the 8.5.x branch, so I thought it was ok. The code changes work on 8.4.0. Now I can manage display for the name.
Comment #25
rwam commentedYou are right, the config should be situated in the module, so I create a new patch to handle this.
But then the 8.5.x branch should be adapted accordingly, shouldn't it?
Comment #26
gábor hojtsyNo, in 8.5.x the config is in the standard profile following #2883813: Move File/Image media type into Standard once Media is stable.
Comment #27
rwam commentedAh ok, I didn‘t noticed that. So please have a look at #25 for 8.4.x.
Comment #28
phenaproxima#25 will only apply to 8.4.x, and is not needed in 8.5.x as @Gábor Hojtsy points out, so I'm changing the branch.
Comment #29
phenaproximaI think the patch looks good. I don't think I can RTBC, but if someone else can...
Comment #30
bdimaggioYep, tested with 8.4.2 and this works just as it should.
Comment #32
larowlanCommitted as 7a8ffbf and pushed to 8.4.x