Problem/Motivation

This issue is postponed on all must-haves from #2834729: [META] Roadmap to stabilize Media Library.

Once the roadmap for the Media Library is complete, we'll need signoffs on the final state as well as a patch to actually mark the module stable.

Proposed resolution

Remaining tasks

Release management checklist

  1. ✅ Security: Known public and private security issues are addressed, including #3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label, #2969678: Integrate Media Library with Content Moderation, etc.
  2. ✅ Data integrity and upgrade path: Known issue addressed with #2981105: Media Library should not modify the media view
  3. ✅ Impact on stable functionality

    Integrations

  4. Maintenance and technical debt
    • ✅ Active maintainers listed with their agreement to maintainer roles documented
    • ✅ Issue queue review

      We've done lots of triage with the maintainers, accessibility reviewers, etc. There are between 20-25 major issues outstanding, and no criticals. About half of the major issues are further accessibility work for the module, some of which will be required before the module is added to the Standard install profile, as described in #2825215: Media initiative: Roadmap. There has been really great progress to reduce the major issue count over the past two months and to improve the accessibility as much as possible before we mark it stable.

      Great work!

  5. API definition and BC policy (mostly ✅, see #39; one remaining fix needed in #3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate)
  6. Core gates

User interface changes

API changes

Data model changes

Release notes snippet

Media Library provides content editors and site builders an interface to visually browse and manage media in their site. It also provides an intuitive modal dialog for reusing media in entity reference fields and text editors. Users with appropriate access can also add new media from directly within the library.

This module was introduced as an experimental core module in Drupal 8.6.0, but is now stable and ready for production use! For sites using the experimental module prior to 8.8.0 there are some important changes:

  • All of Media Library's styling and associated CSS classes were moved into Classy and Seven. Classy provides a very minimal amount of basic layout for the media library; Seven provides a more complete experience. If you are not using Seven to display the media library, you may need to add code to your theme to ensure that the moved CSS classes are applied to the media library in the correct places. See https://www.drupal.org/node/3089245 and https://www.drupal.org/node/3089300 for more information, and core/themes/seven/seven.theme for examples of how to apply the required CSS classes in your theme.
  • The "add forms" provided by the Media Library module now have two different form IDs. Previously, both shared the same form ID (media_library_add_form). This is now their shared base form ID. Their individual form IDs are now media_library_add_form_upload and media_library_add_form_oembed. Any code altering either of these forms may be adjusted, and any custom form extending \Drupal\media_library\Form\AddFormBase will need to be changed as well. See https://www.drupal.org/node/3089217 for more information.
  • The media library's administrative grid interface is no longer exposed at /admin/content/media. That path now shows the administrative table listing of media items, as it does without Media Library installed. The grid interface is linked from there, and exposed at /admin/content/media-grid. An update path for this is provided; no further action is needed from existing sites.
CommentFileSizeAuthor
#121 interdiff-3082690-96-121.txt316 bytesphenaproxima
#121 3082690-121-D9.patch118.03 KBphenaproxima
#98 3082690-96-D9.patch118.13 KBJeroenT
#96 3082690-96.patch118.02 KBoknate
#96 interdiff--3082690--87-96.txt281 bytesoknate
#87 3082690-87.patch118.03 KBphenaproxima
#81 interdiff-3082690-77-81.txt742 bytesphenaproxima
#81 3082690-81.patch119.49 KBphenaproxima
#77 interdiff-3082690-76-77.txt943 bytesphenaproxima
#77 3082690-77.patch119.24 KBphenaproxima
#76 interdiff-3082690-70-76.txt11.83 KBphenaproxima
#76 3082690-76.patch119.05 KBphenaproxima
#70 interdiff-3082690-62-70.txt2.61 KBphenaproxima
#70 3082690-70-review-only-do-not-test.txt42.13 KBphenaproxima
#70 3082690-70.patch119.94 KBphenaproxima
#65 ML-Umami-dialog-add.png56.38 KBeffulgentsia
#65 ML-Bartik-dialog-add.png38.22 KBeffulgentsia
#65 ML-Seven-dialog-add.png34.61 KBeffulgentsia
#62 3082690-62-review-only-do-not-test.patch39.73 KBphenaproxima
#62 3082690-62.patch117.97 KBphenaproxima
#61 3082690-61.patch117.92 KBphenaproxima
#59 3082690-58-2.png231.68 KBphenaproxima
#59 3082690-58-1.png578 KBphenaproxima
#55 ML-widget-populated-after.png43.57 KBeffulgentsia
#55 ML-widget-populated-before.png42.02 KBeffulgentsia
#55 ML-dialog-table-after.png53.08 KBeffulgentsia
#55 ML-dialog-table-before.png54.45 KBeffulgentsia
#55 ML-dialog-grid-after.png53.56 KBeffulgentsia
#55 ML-dialog-grid-before.png49.72 KBeffulgentsia
#55 ML-dialog-add-after.png57.15 KBeffulgentsia
#55 ML-dialog-add-before.png45.1 KBeffulgentsia
#55 ML-dialog-empty-after.png45.01 KBeffulgentsia
#55 ML-dialog-empty-before.png55.97 KBeffulgentsia
#55 ML-widget-empty-after.png27.07 KBeffulgentsia
#55 ML-widget-empty-before.png28.28 KBeffulgentsia
#52 interdiff-3082690-49-52.txt4.16 KBphenaproxima
#52 3082690-52.patch113.7 KBphenaproxima
#52 3082690-52-review-only-do-not-test.patch39.73 KBphenaproxima
#49 3082690-49.patch109.53 KBphenaproxima
#49 3082690-49-review-only-do-not-test.patch35.56 KBphenaproxima
#46 3082690-46.patch35.55 KBphenaproxima
#45 3082690-45-REROLL.patch32.6 KBbnjmnm
#35 interdiff-3082690-27-35.txt10.94 KBphenaproxima
#35 3082690-35.patch36.41 KBphenaproxima
#27 interdiff_25-27.txt37.97 KBbnjmnm
#27 3082690-27.patch33.12 KBbnjmnm
#27 changes-exclusive-to-this-issue_27.txt16.67 KBbnjmnm
#25 interdiff_18-25.txt5.51 KBbnjmnm
#25 3082690-25.patch23.72 KBbnjmnm
#23 3082690-interdiff--8-18.txt3.09 KBoknate
#18 3082690-18.patch18.21 KBphenaproxima
#8 3082690-8.patch15.4 KBphenaproxima
#7 3082690-7.patch11.16 KBphenaproxima
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
phenaproxima’s picture

Discussed with @seanB about the @internal vs. @api stuff.

Short answer: everything should be @internal except for the following:

  • \Drupal\media_library\Form\AddFormBase
  • \Drupal\media_library\MediaLibraryOpenerInterface

These two things were meant to be implemented and/or extended by external code, but they were marked internal because Media Library is (currently) experimental. When we're stable, we're comfortable explicitly opening these up to extension.

It seems reasonable to go stable with the smallest possible "API surface", and open things up later as needed. It's a lot easier to do that than to open everything up at once and have to rope things off later.

phenaproxima’s picture

Here is an initial patch which updates MAINTAINERS.txt, the info file, and all PHP classes with the agreed-upon @internal or @api designation.

phenaproxima’s picture

Because it was "virtually committed" (i.e., marked postponed), I'm rolling #3035994: Add a Media Library views template to Seven theme to change to order of the views header and exposed filters into this patch, since no further work is expected on that one.

phenaproxima’s picture

phenaproxima’s picture

Currently, the patch lists @seanB and myself as maintainers of Media Library. I can't speak for @seanB, but to fulfill governance requirements: I formally accept maintainership of this module.

phenaproxima’s picture

Issue summary: View changes
seanB’s picture

Issue summary: View changes

I too, formally accept maintainership of the module.

phenaproxima’s picture

This includes the changes from #3038489-33: Add a Media library views template to Seven theme to add wrapper around the grid rows, which was reviewed and approved by @lauriii today in a Zoom call. One to go!

phenaproxima’s picture

webchick’s picture

To help expedite the final reviews of this patch, once we get to the finish line, would you be able to please also upload a patch that is only the changes not already reviewed/"committed" by other issues? That would expedite the final reviews to ensure we're only focusing on new stuff.

xjm’s picture

Aye, having both an "interdiff" from those and a complete patch would be handy.

oknate’s picture

FileSize
3.09 KB

Interdiff from #8 to #18.

oknate’s picture

#3038489: Add a Media library views template to Seven theme to add wrapper around the grid rows has landed. So my interdiff posted in #23 might be off. But for a good reason!

bnjmnm’s picture

#18 was the first patch in this issue that tests were run on. The three fails there were all related to tests that run on modules marked stable. @phenaproxima and I worked on addressing these.
StableLibraryOverrideTest and StableTemplateOverrideTest due to the module's theming happening in Seven instead of Stable. Media library was added to the $<thing>toSkip variable in both instances.
DefaultConfigTest was addressed by moving optional config from the module to the Standard profile.

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

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

bnjmnm’s picture

This adds the changes from #3082690: Mark Media Library as a stable core module, making this ready for review.

changes-exclusive-to-this-issue_27.txt is a diff with only the changes that occurred in this issue.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

My eyes are not dry, friends. Cry if you feel it!

This has been a very, very long road for us to walk together. From the Berlin sprint of 2016, to the Barcelona sprint of 2018, to 2019 Dev Days in Cluj...and all the things we did in between. Wim said it well in his blog post about the CKEditor embedding stuff: the Media Initiative has a massive scope, and this feels like, at least to some extent, the culmination of it. There's more to do, but look what have already done!

I'm not listed officially as the initiative coordinator for Media, and that's fine with me, but it's the role I've been playing for at least the last couple of releases. I'm not even very good at herding cats; I prefer to code. But since this was the position in which I found myself, I have to say that I could NOT, ever, not even in my wildest dreams, have asked for a better set of people to labor on this thing with. You all know who you are.

So: thanks for everything! This has been, in a word, epic.

Now, that said: I'm going to take a hiatus from this initiative between 8.9 -> 9.0, when no features are being added, but I will be back in 9.0 for the final push -- migrating File and Image fields to Media. I can still review issues and do some Media stuff, and I'll be in Slack as always, but I will not be working to push the initiative forward until 9.0 is released. (Keep me in MAINTAINERS.txt.)

Let me do now what I do best: prematurely mark this issue RTBC on the assumption that tests will pass. Cheers, hugs, and victory to all!

phenaproxima’s picture

Issue tags: -Needs change record

Draft change record written: https://www.drupal.org/node/3087431

svettes’s picture

I'm not crying, YOU'RE CRYING, Adam!!!
crossing fingers, feels like a lock. You've done an amazing job leading this as far as you have, and you can be very proud of all the hard work.
Thanks Adam, and all those involved for your dedication and the *years* of work that went into this.
Me: < sings For they're some jolly good people >

The last submitted patch, 7: 3082690-7.patch, failed testing. View results

The last submitted patch, 8: 3082690-8.patch, failed testing. View results

webchick’s picture

I just went through and did a thorough walkthrough of this module's functionality at #3034242-58: Hide "Save and insert" and "Additional selected media" from users by default. (As well as over and over, throughout the past couple of weeks.)

I really think this feature will be a game-changer for Drupal. It's baseline expected functionality for a content management system today, and the amazing work that's gone in over the years for usability, design, and flow has been nothing short of astounding. GREAT work, everyone!!

Product management: Signing off! *salutes*

effulgentsia’s picture

I agree that this module is ready to be stable. Great work, everyone!

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
36.41 KB
10.94 KB

Refined the @internal stuff in the grand Layout Builder tradition, and added media_library.api.php at Wim's suggestion, to document how the module is put together.

lauriii’s picture

As part of my review, I realized that media library is adding a lot of classes in templates, form API and preprocess functions. We should move these to Classy: #3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#35: that looks great! Having worked on various parts of Media Library but definitely not all of it, I think the documentation you wrote for media_library.api.php captures the architecture, scope and intent clearly. It helps future Drupal core contributors understand the module, and hence improves its maintainability and debuggability. 👏👍

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks all!

As discussed Friday, we need #3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate included in-scope here. So marking NW for that and also adding it to the roadmap issue.

xjm’s picture

+++ b/core/MAINTAINERS.txt
@@ -283,6 +283,10 @@ Media
+Media Library
+- Sean Blommaert 'seanB' https://www.drupal.org/u/seanb
+- Adam Globus-Hoenich 'phenaproxima' https://www.drupal.org/u/phenaproxima
+

Can we add this as an indented sub-section under Media? See the database API and Migrate for similar examples.

I read over all the high-level API documentation as well as the public and internal API definition as documented on individual classes, and it all looks great! Nice work.

xjm’s picture

Issue summary: View changes

I've read over the module description, the module handbook page, and the hook_help() as well, and all look pretty good. A few small points of feedback:

  1. Are the hook_help() explanations regarding the original media listing view still completely accurate following the fix in #2981105: Media Library should not modify the media view?

  2. Minor point regarding:

          $output .= '<li>' . t('Both the table-style and grid-style interfaces are regular views and can be customized via the Views UI, including sorting and filtering. This is the case for both the administration page and the modal dialog.') . '</li>';
    

    Typically, we'd conditionally link Views UI here (either the UI itself, or the help documentation for it) if Views UI is enabled and the user has access to it.

  3. The wording here is a little awkward:

    which fields are displayed (including which image style is used for images) can be customized by configuring the "Media library" view mode for each of your <a href=":media-types">media types</a>.
    

    I'd suggest instead:

    the fields that are displayed [...rest the same]
    

    The same change should be made in the last two bullets.

xjm’s picture

Issue summary: View changes
xjm’s picture

I think this should also have a small release note mentioning the unusual shuffling of frontend code we did compared to most experimental modules when they become stable. (It's already tagged for it.) The release note should also link the relevant change records for those things -- maybe we should link them separately at the bottom of the main CR for this issue?

xjm’s picture

Issue summary: View changes

The accessibility gates have also been passed based on the maintainers' feedback on #2834729: [META] Roadmap to stabilize Media Library.

phenaproxima’s picture

Issue tags: +Needs reroll
bnjmnm’s picture

Rerolled.

phenaproxima’s picture

Addressed all of @xjm's feedback in #39 and #40. Leaving NW until #3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate is in.

Regarding this:

Are the hook_help() explanations regarding the original media listing view still completely accurate following the fix in #2981105: Media Library should not modify the media view?

The answer is yes, they are. That issue did not make any user-facing changes.

phenaproxima’s picture

Note that, per @lauriii in #3087456-80: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate, all of Media Library's templates should be added to Stable in this patch.

phenaproxima’s picture

Issue tags: +Needs reroll

#3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate has been marked fixed by @lauriii, so this needs a reroll on top of that. From here on out, we'll post two patches each time we update: the "real" patch we will commit in this issue, and a review-only ("-do-not-test.patch") patch that contains only the things which are changing in this issue.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
35.56 KB
109.53 KB

And, reroll complete.

phenaproxima’s picture

Issue tags: -Needs reroll

See above.

Status: Needs review » Needs work

The last submitted patch, 49: 3082690-49.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
39.73 KB
113.7 KB
4.16 KB

This should bring the tests back from the red.

phenaproxima’s picture

+++ b/core/themes/seven/templates/views-view-unformatted--media-library.html.twig
@@ -0,0 +1,35 @@
+      set row_classes = [
+        default_row_class ? 'views-row',
+        'media-library-item',
+        'media-library-item--grid',
+    ]

Minor indentation fail. Can be fixed on commit, or in the next iteration of this patch. Whichever comes first.

phenaproxima’s picture

effulgentsia’s picture

Part of what this patch does is moves a bunch of CSS from the module to Seven.

So I was curious how that affects what the Media Library looks like in Bartik. I created two separate fresh installs on my local machine: one with 8.8.x HEAD, one with #52 applied. For each one, I enabled Media Library, and then added a Media field to the "Default comments" comment type. Then I created an Article node, and from there could see what the Media widget and dialog looks like from a node page that can be commented on, which is themed in Bartik.

Here are the before and after screenshots.

phenaproxima’s picture

Two options come to mind about the above. Both are based upon the assumption that Bartik is closed, internal, and can be changed at any time.

  1. Commit this as-is, and open an issue to copy the styling, templates, and support code from Seven over to Bartik after stable. The main benefit of this approach is that it doesn't increase the size of this patch.
  2. Copy the styling, templates, and support code from Seven to Bartik in this issue (with a dedicated test for Bartik, like the new MediaLibrarySevenTest, to ensure no functionality is broken), and fine-tune Bartik later if needed. This will ratchet up the size of the patch significantly, but will probably fix most of the visual regressions now.
lauriii’s picture

Thanks for generating the screenshots @effulgentsia! We could work around the problem by copying the Seven styles to Bartik but I don't think it's the right solution. The designs are Seven specific, and since the styles are located in Bartik, they wouldn't be modifiable by other admin themes. For example, Claro would run into this issue pretty soon in #3062751: Media and media library. Also, this would only solve the problem in the scope of Bartik, and every other frontend theme would run into the same problem.

I think the right solution would be to solve #2195695: Admin UIs on the front-end are difficult to theme but that will likely take some time.

webchick’s picture

Grid that's not a grid

This one is particularly un-good. It looks like after this patch (or the other patch; I get all the patches confused), all themes are starting from the standpoint of “two copies of your thumbnails, and also not in a grid despite the button says ‘grid’” which seems... not... awesome... we might be taking "lack of theme opinionatedness" a bit too far here. :\

phenaproxima’s picture

FileSize
578 KB
231.68 KB

Spoiler alert: I applied the patch in #52, then copied all of the Seven stuff into Bartik. It looked great; screenshots attached.

So, if you ask me, copying all the Seven stuff into Bartik is a fine workaround. It's not the "right" solution, as @lauriii says, but it might be the best we can do in the short term.

Another option is for us to keep media_library.theme.css in the Media Library module, but go over it with a fine-tooth comb and only keep styles which keep the library looking reasonably good in most themes by default. But that sort of undermines the point of #3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate.

lauriii’s picture

I discussed with @effulgentsia and @phenaproxima about #55 and we agreed that copying Seven styles to Bartik and Umami would be just a workaround solving this problem in our own product. This would still leave all other themes suffering from this. Also moving the styles to Classy would not be good because we know we want to modify these styles in the future. We decided that as a step to try to improve the UX on Umami and Bartik, we will add some basic layout rules to Classy to make the layout look less broken: #3089743: [PP-1] Media library layout is broken on Bartik and Umami. This should be done prior to Stable because we shouldn't be adding this type of rules after Media Library has been marked stable.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
117.92 KB

The last submitted patch, 61: 3082690-61.patch, failed testing. View results

The last submitted patch, 62: 3082690-62.patch, failed testing. View results

effulgentsia’s picture

Here's some screenshots of the dialog right after you upload an image and before you've filled in the alt text.

Notice how in Bartik and Umami, the image is shown twice, and the Remove button is shown twice.

phenaproxima’s picture

Yup, that's a pre-existing condition, and covered by #2987921: Media Library add form should suppress extraneous components of image fields using a form alter, not CSS (as commented in the CSS):

/* @todo Remove in https://www.drupal.org/project/drupal/issues/2987921 */
.media-library-add-form__source-field .file,
.media-library-add-form__source-field .button,
.media-library-add-form__source-field .image-preview,
.media-library-add-form__source-field .form-type-managed-file > label,
.media-library-add-form__source-field .file-size {
  display: none;
}

This is done because, when uploading new image media, we need to hide all parts of the source field (which is an image field), except for the alternative text. We currently do that with CSS, and have for a long time.

Because we intend to remove it later and the upload is still usable, albeit ugly, I don't think this is something we should be fixing in Classy. The compromise I'd suggest, if anything, is to fix this temporarily in Bartik (just copy that entire CSS rule into it).

Another option would be for Media Library to implement hook_field_widget_WIDGET_TYPE_form_alter() and suppress those parts. But again, I don't think that should block stable, and it should be done in #2987921: Media Library add form should suppress extraneous components of image fields using a form alter, not CSS.

The last submitted patch, 62: 3082690-62.patch, failed testing. View results

webchick’s picture

With strong gnashing of teeth, I'm gonna say that the screenshots in #65 are acceptable to ship with. There's a known bug, with a patch, and once it's fixed, all of the themes affected will go back to normal. (As opposed to the grid styling, which required all themes to do a thing to "fix" it.)

So count this as re-PM sign-off of this awesome initiative!

xjm’s picture

I asked @phenaproxima if we can fold #3087126: Media Library "select all" checkbox does not have proper margin in RTL styling into this issue so that RTL sites look OK too. It sounds like an easy/small thing to add. (Hopefully.)

phenaproxima’s picture

Okay. Added some workarounds to MediaLibraryTest to stabilize testAdministrationPage(), which is known to be flaky on local machines. Also rolled in a fix for #3087126: Media Library "select all" checkbox does not have proper margin in RTL styling, which I tested manually, and will close that issue.

phenaproxima’s picture

Issue summary: View changes
Issue tags: -Needs release note

Added a release note. I know it's simplistic, but I'm not sure what else to say and couldn't find any other examples.

phenaproxima’s picture

Issue summary: View changes

Expanded the release note a bit, based on what Layout Builder had in 8.7.0.

Status: Needs review » Needs work

The last submitted patch, 70: 3082690-70.patch, failed testing. View results

lauriii’s picture

+++ b/core/modules/media_library/css/media_library.module.css
@@ -1,118 +1,3 @@
 /**
 * @file media_library.module.css
 */

This CSS file is now empty. Should we just remove this and the library?

effulgentsia’s picture

  1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -22,12 +25,9 @@
    + * @api
    +++ b/core/modules/media_library/src/MediaLibraryOpenerInterface.php
    @@ -18,6 +18,8 @@
    + * @api
    

    Looks like we generally don't use an @api annotation anywhere in core/modules or core/lib, despite https://www.drupal.org/core/d8-bc-policy still mentioning it. I only see one usage of it in all of 8.8.x: and that's in ConfigEntityUpdater::update(), which maybe is a remnant of an earlier time when we used it more. I'd suggest removing it from this patch, and finding or opening an issue to discuss whether we want to start using this annotation.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTest.php
    @@ -44,7 +44,9 @@ class StableLibraryOverrideTest extends KernelTestBase {
    -  protected $librariesToSkip = [];
    +  protected $librariesToSkip = [
    +    'media_library/style',
    +  ];
    +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableTemplateOverrideTest.php
    @@ -24,6 +24,7 @@ class StableTemplateOverrideTest extends KernelTestBase {
       protected $templatesToSkip = [
         'views-form-views-form',
    +    'media--media-library',
       ];
    

    Do we still need this?

  3. +++ b/core/themes/seven/templates/views-view--media_library.html.twig
    +++ b/core/themes/seven/templates/views-view-unformatted--media-library.html.twig
    

    Let's move these to the media_library subdirectory.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
119.05 KB
11.83 KB

This won't fix the test failures, but...

Addressing #74:

  1. Yup. I removed the library and the adjusted the related libraries-extend stuff in Seven and Classy.

Addressing #75:

  1. ✅Removed both annotations.
  2. ✅In light of #74, I don't think so. Removed.
  3. ✅Done.
phenaproxima’s picture

@lauriii pointed out that we were still attaching the seven/media_library library in views-view-unformatted--media-library.html.twig. Now that we're using libraries-extend, there's no need for that, so I removed it. I also had Seven override classy/media_library so as to ensure that the layout styling in Classy doesn't conflict with the styling in Seven, which totally supersedes it.

lauriii’s picture

I don't have any more concerns. Good job on addressing all the feedback 👏

The last submitted patch, 76: 3082690-76.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 77: 3082690-77.patch, failed testing. View results

phenaproxima’s picture

I suspect this will fix the tests. I don't know why MediaLibraryTest::testAdministrationPage() is having such trouble with, y'know, pressing a button to submit a form (and, weirdly, only in that one test...and even more weirdly than that, only failing consistently on testbot). But, this workaround seemed to help when I tried it in a burner issue.

MediaLibraryTest is such a terrible, horrible, no-good beast anyway that it doesn't seem so awful to put workarounds in it for now in the name of marking this module stable. 🤷‍♂️

seanB’s picture

Status: Needs review » Reviewed & tested by the community

All tests are green and as far as I can see all concerns are addressed and the work in all child issues has been merged in the main patch. Marking this RTBC. Great work everyone, thank you! ❤

phenaproxima’s picture

Issue summary: View changes

Expanding the release notes with info about disruptive changes.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

I sat with @lauriii and we went through the many arcane changes from the several theme blockers. The pretty spreadsheet was also a big help!

There are a few out-of-scope cleanups to small things like inline comments, but they're trivial enough that I'm not concerned about it. The only things I'm still hesitant about are in the new MediaLibrarySevenTest:

  1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibrarySevenTest.php
    @@ -0,0 +1,42 @@
    +/**
    + * @group media_library
    + */
    

    There's no documentation anywhere on this test class... At a minimum there needs to be a docblock here.

  2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibrarySevenTest.php
    @@ -0,0 +1,42 @@
    +class MediaLibrarySevenTest extends MediaLibraryTest {
    

    This will run all the methods of this huge, very unstable, oft-failing test again... are we doing that on purpose? And... do we have to? I still get emails literally every night from at least one of the twenty-odd test environments failing on this test.

    It's good that it has test coverage, don't get me wrong. I'm just worried about this failing twice as much. In some ways it can be considered a "new" random fail too... even though it should also be fixed by #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest) etc., it makes it twice as bad until then.

  3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibrarySevenTest.php
    @@ -0,0 +1,42 @@
    +   * {@inheritdoc}
    ...
    +   * {@inheritdoc}
    

    Both @lauriii and I initially thought this was a copypasta mistake. I guess these are indeed in MediaLibraryTest and therefore both the @inheritdoc and the subclassing are intentional. But... erk.

  4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -128,16 +128,16 @@ public function testAdministrationPage() {
    -    $this->waitForText('Dog');
         $this->waitForNoText('Turtle');
    +    $assert_session->pageTextContains('Dog');
         $page->selectFieldOption('Media type', 'Type Two');
         $page->pressButton('Apply filters');
    -    $this->waitForNoText('Dog');
         $this->waitForText('Turtle');
    +    $assert_session->pageTextNotContains('Dog');
    

    Why?

  5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,14 +145,17 @@ public function testAdministrationPage() {
    -    $this->submitForm([], 'Delete');
    +    // For reasons that are not clear, deleting media items by pressing the
    +    // "Delete" button can fail (the button is found, but never actually pressed
    +    // by the Mink driver). This workaround allows the delete form to be
    +    // submitted.
    +    $assert_session->elementExists('css', 'form')->submit();
    

    Should this have had its own issue?

  6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,14 +145,17 @@ public function testAdministrationPage() {
    -    $select_all = $assert_session->waitForField('Select all media');
    -    $this->assertNotEmpty($select_all);
    +    $select_all = $this->waitForFieldExists('Select all media');
    

    Out of scope?

  7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -2247,6 +2249,9 @@ protected function openMediaLibraryForField($field_name, $after_open_selector =
    +    // The "select all" checkbox should never be present in the modal.
    +    $assert_session->elementNotExists('css', '.media-library-select-all');
    

    New assertion?

NW for the first point; others are open questions.

Sooooo close!

phenaproxima’s picture

  1. Delaying any further action on this until a decision is in about the next point.
  2. I'm pretty ambivalent about this test. I would not shed any tears if we deleted it. It's worth noting that we did move a lot of front-end support code into Seven, so it makes sense that we might want to test the media library in Seven to assure we didn't break anything there. OTOH, if MediaLibraryTest is already random failing a lot, it stands to reason that this will increase those failures. And, in light of the upcoming split and refactor of MediaLibraryTest, it might not really make sense to effectively duplicate all of the coverage in Seven. So, I leave the decision of whether to keep it or toss it, up to you.
  3. These overridden methods are there in order to assert specific changes that Seven makes. If we keep MediaLibrarySevenTest, we should keep them.
  4. The original code is a little unclear, logically speaking. The page already contains "Dog", so there's no real reason to "wait" for it. Then we change the filters so that "Dog" won't appear anymore but, after a wait, "Turtle" will, at which point there's no real reason to "wait" for "Dog" to be gone (when filtering by media types, "Dog" will not be there if "Turtle" is). I'm open to changing this back if you feel strongly about it.
  5. I don't think so. For reasons that are unclear, it only started happening in this issue in the first place, out of nowhere, both on testbot and my local machine (and more consistently on testbot than on my local machine, for some reason). This failure did not happen in either of the front-end dragons which preceded this patch, and there is just no way I can see that any code we are adding in this issue would cause it. (If I'm missing something, please help me see it!) So I used this workaround to make it pass. That said, this is indeed a temporary and apparently stable workaround in a nightmare test that is aching to be refactored, so I don't think it warrants its own blocking issue.
  6. Yes, we can change that back. I prefer it this way, personally, since it's more brief, but it won't hurt anything in the old format.
  7. Yes. @lauriii and I made a necessary JavaScript improvement as part of moving things to Seven, and that fix caused the "Select all" checkbox to never be created in the widget. Previously, it was created in the widget, but suppressed with CSS. The new way is as it truly should be (hiding the checkbox with CSS was always a crappy hack), so we replaced the previous assertions (which verify that the checkbox is present but not visible) with this one (which verifies the checkbox doesn't actually exist at all). So this is being updated for correctness. We could remove the assertion entirely, but then we are removing coverage.
phenaproxima’s picture

I discussed the fate of MediaLibrarySevenTest with @xjm. On the one hand, she didn't want to remove valuable added test coverage. On the other hand, she didn't want to bump up the number of random failures. I can certainly appreciate that conundrum!

We agreed to rip MediaLibrarySevenTest out of this patch completely, and open a follow-up, postponed on the refactor of MediaLibraryTest, to restore it later. Since the planned refactoring of MediaLibraryTest will split it into several smaller, more stable tests, MediaLibrarySevenTest will eventually just be a short, well-scoped test that runs through the parts of the media library which are actually touched by Seven.

I will work to deliver a green-on-all-backends patch within the next few hours. Given the time constraint, @xjm authorized me to set that directly back to RTBC when it's ready.

phenaproxima’s picture

phenaproxima’s picture

Issue tags: +Needs followup

Tagging for the follow-up discussed in #86.

phenaproxima’s picture

Opened #3090983: [PP-2] Test Seven's changes to Media Library to restore MediaLibrarySevenTest as a proper, tightly scoped, well-written test covering Seven's modifications to Media Library.

And, as per discussion with @xjm, restoring RTBC on the assumption that all backends will be green.

xjm’s picture

Issue summary: View changes

Going on through my checklist...

So there are a few testing followups that are things we really should do for Media Library test, because it is very. Very very. Disruptive at the moment. These don't need to block marking the module stable, because with @phenaproxima's update in #87, it's exactly as disruptive before or after stable.

That said, I'm adding the issues to the checklist in the summary. If it's possible to fix them before 8.8.0, that would be really great. I'd also consider them blockers for adding the module to Standard so I'll update that roadmap too (if someone didn't already).

xjm’s picture

Issue summary: View changes

My docs feedback from #40 has been addressed so checking off docs!

xjm’s picture

Issue summary: View changes

CKEditor integration has been addressed in the numerous must-haves in #2801307: [META] Support WYSIWYG embedding of media entities, and Content Moderation addressed in #2969678: Integrate Media Library with Content Moderation.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

Checking off more items that are already addressed... which is all of them!

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs release manager review +Needs reroll

NooooOOOOoooo it doesn't apply... 😿

Reroll please? 😺

oknate’s picture

Here's a reroll. The only difference was in the media_library.libraries.yml. The interdiff is a little weird, since it's a diff between the two patches.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Ah, I see, the reroll was due to jQuery UI deprecations that were likely committed during DrupalCon Amsterdam. (We were depending on Sortable.) RTBC on the assumption that tests will pass.

phenaproxima’s picture

Verified that the changes needed to make #96 apply to 9.0.x are present in JeroenT's patch. Thanks!

xjm’s picture

We can't use PHP 7.1 test envs. for 9.0.x; I'll queue the 7.3 ones instead.

xjm’s picture

+++ b/core/modules/media_library/media_library.info.yml
@@ -1,7 +1,7 @@
 name: 'Media Library'
 type: module
 description: 'Enhances the media list with additional features to more easily find and use existing media items.'
-package: Core (Experimental)
+package: Core
 version: VERSION
 dependencies:

So this is where the patch failed to apply to 9.0.x, on the context lines. In 8.8.x it looks like:

+++ b/core/modules/media_library/media_library.info.yml
@@ -1,7 +1,7 @@
 name: 'Media Library'
 type: module
 description: 'Enhances the media list with additional features to more easily find and use existing media items.'
-package: Core (Experimental)
+package: Core
 version: VERSION
 core: 8.x
xjm’s picture

Adding credits for reviewers on this issue for people who have already commented.

xjm credited DyanneNova.

xjm credited Gábor Hojtsy.

xjm credited chr.fritsch.

xjm credited marcoscano.

xjm credited webflo.

xjm’s picture

Moe credits for contributors for everything from the initial prototype to the roadmap itself...

xjm credited Berdir.

xjm credited FeyP.

xjm credited annagaz.

xjm credited cboyden.

xjm credited peterx.

xjm credited rainbreaw.

xjm credited shaal.

xjm’s picture

More credits...

xjm credited jan.stoeckler.

xjm’s picture

xjm’s picture

The ES6 transpilation for 9.0.x seems to be out of sync:

yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/xjm/git/maintainer/core/modules/media_library/js/media_library.view.es6.js
[16:23:23] '/Users/xjm/git/maintainer/core/modules/media_library/js/media_library.view.es6.js' is being checked.
[16:23:23] '/Users/xjm/git/maintainer/core/modules/media_library/js/media_library.view.es6.js' is not updated.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/xjm/git/maintainer/core/modules/media_library/js/media_library.view.es6.js
[16:23:24] '/Users/xjm/git/maintainer/core/modules/media_library/js/media_library.view.es6.js' is being checked.
[16:23:25] '/Users/xjm/git/maintainer/core/modules/media_library/js/media_library.view.es6.js' is not updated.
error Command failed with exit code 1.

The 8.9.x/8.8.x version of the patch passes this check.

phenaproxima’s picture

Ran yarn build:js against #96 on 9.0.x.

  • xjm committed e0ae963 on 9.0.x
    Issue #3082690 by phenaproxima, bnjmnm, oknate, JeroenT, effulgentsia,...

  • xjm committed 946835f on 8.9.x
    Issue #3082690 by phenaproxima, bnjmnm, oknate, JeroenT, effulgentsia,...

  • xjm committed 9239091 on 8.8.x
    Issue #3082690 by phenaproxima, bnjmnm, oknate, JeroenT, effulgentsia,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

COMMITTED TO 9.0.x, 8.9.x, AND 8.8.x! WOOOOOOOO!

Excessive and profuse thanks to everyone who pulled together and made this happen in this release despite the many obstacles that came up for us. This is a huge milestone for the Media Initiative. Extra thanks to @phenaproxima for long-suffering. :)

Next step: STANDARD.... https://www.youtube.com/watch?v=-bzWSJG93P8

xjm’s picture

DEFINITELY a highlight of this release. Also the release note about the theme shuffles should go in the release notes.

pookmish’s picture

I believe this is the issue an error has come from:
User error: "0" is an invalid render array key in Drupal\Core\Render\Element::children()

I found that in the function seven_form_media_library_add_form_upload_alter() there is an incorrect attributes key.

$form['attributes']['class'][] = 'media-library-add-form--upload';

should be $form['#attributes']['class'][] = 'media-library-add-form--upload';

My database logs are full of this error now.

oknate’s picture

@pookmish. Nice find. Let's move this to a follow-up issue.

I added one: #3094397: Attributes key missing hash or pound sign in seven

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.