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

Comments

Manuel Garcia created an issue. See original summary.

dawehner’s picture

Is there a change record we could/should update in case this gets in?

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!
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

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Status: Reviewed & tested by the community » Needs work

I forgot about the es6 files... patch coming soon

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
434 bytes
2.36 KB

Renaming 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.

Manuel Garcia’s picture

Issue summary: View changes
Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

Latest 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.

larowlan’s picture

Can 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.

Manuel Garcia’s picture

Sure thing @larowlan, how about something like:

The libraries defined by media, previously media_form and media_type_form have had the media_ prefix removed.

These libraries are attached to MediaForm::form() and MediaTypeForm::form().

$form['#attached']['library'][] = 'media/media_form'; becomes $form['#attached']['library'][] = 'media/form';

$form['#attached']['library'][] = 'media/media_type_form'; becomes $form['#attached']['library'][] = 'media/type_form';

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Just 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).

Manuel Garcia’s picture

For the header... how about

If your code interacts with MediaForm or MediaTypeForm...
phenaproxima’s picture

Issue tags: -D8Media +Media Initiative

Re-tagging.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record updates, +Needs change record

Media 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.

Manuel Garcia’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record updates, -Needs change record

Added the change record and edited the previous change record to include this section:

If your code interacts with MediaForm or MediaTypeForm...

Libraries provided by media module have been renamed, please see the change record for this: https://www.drupal.org/node/2889470

https://www.drupal.org/node/2863992/revisions/view/10530717/10538356

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I'm happy with that. Full steam ahead!

Wim Leers’s picture

lgtm!

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

I 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: Essentials - first round of improvements in core) so there is no certainty whatsoever for contrib.

Manuel Garcia’s picture

Thanks @Gábor Hojtsy - er.. did you forget to actually commit the patch?

  • Gábor Hojtsy committed 9548294 on 8.4.x
    Issue #2887269 by Manuel Garcia, phenaproxima, larowlan, xjm: Remove...