Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
media.libraries.yml declares its libraries as:
media_form:
version: VERSION
js:
js/media_form.js: {}
dependencies:
- core/drupal.form
media_type_form:
version: VERSION
js:
js/media_type_form.js: {}
dependencies:
- core/drupal.form
It is not necessary to prefix them with media_ as you already attach them as 'media/LIBRARY'
Proposed resolution
Remove the prefixes.
Remaining tasks
Review the attached patch.
User interface changes
None
API changes
None
Data model changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 2887269-5.patch | 2.36 KB | Manuel Garcia |
| #5 | interdiff.txt | 434 bytes | Manuel Garcia |
| media-libraries.patch | 1.93 KB | Manuel Garcia | |
Comments
Comment #2
dawehnerIs there a change record we could/should update in case this gets in?
Comment #3
naveenvalechaThanks!
Here's the CR, we could mention about it https://www.drupal.org/node/2863992 if required, but I think it's okay even without it but not sure.
//Naveen
Comment #4
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedI forgot about the es6 files... patch coming soon
Comment #5
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRenaming the es6 files accordingly as well.
About the CR... I suppose we could mention it, but I doubt these two files could be used anywhere other than within the media form and media type forms.
Comment #6
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #7
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedLatest change is just to account for #2818825: Rename all JS files to *.es6.js and compile them (see https://www.drupal.org/node/2815083), safe to go back to RTBC.
Comment #8
larowlanCan we get a draft of the change record addition here, so that it can be added to https://www.drupal.org/node/2863992 by a committer when it goes in.
Comment #9
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedSure thing @larowlan, how about something like:
Comment #10
xjmJust a note that since Media is technically in beta, we should avoid unnecessary BC breaks. Media was added as beta experimental specifically to provide API stability for contrib immediately upon commit; otherwise we would have committed it as alpha months earlier. Experimental policy reference: https://www.drupal.org/core/experimental#beta
Since it's only been in the dev branch for a month and never in any release, it might be okay, but let's have a quick think about whether it's necessary, and do a quick survey of the Media contrib modules to see which might be affected by this?
The potential CR text looks good; let's also propose a header for it for https://www.drupal.org/node/2863992. I'm adding this issue to that CR now (we can do that even while this is NR).
Comment #11
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedFor the header... how about
Comment #12
phenaproximaRe-tagging.
Comment #13
phenaproximaMedia is beta experimental, but for me the key word is (at least before 8.4.0-alpha1) "experimental". Therefore, I think it's appropriate to make this change now. The patch looks good to me and is RTBC as far as I'm concerned.
That said, we should add a change record for this issue specifically, and we should link https://www.drupal.org/node/2863992 to that change record. So I'm marking this as needing work for those two action items.
Comment #14
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAdded the change record and edited the previous change record to include this section:
https://www.drupal.org/node/2863992/revisions/view/10530717/10538356
Comment #15
phenaproximaI'm happy with that. Full steam ahead!
Comment #16
Wim Leerslgtm!
Comment #17
Gábor HojtsyComment #18
Gábor HojtsyI agree the WTF factor of this overweighs the very minor API change being done here especially that nothing relies on the media module given that we are still in the zone where it may be removed from core (as per roadmap in #2825215: Media initiative: Roadmap) so there is no certainty whatsoever for contrib.
Comment #19
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @Gábor Hojtsy - er.. did you forget to actually commit the patch?