Problem/Motivation

I was trying to build #2877383: Add action support to Media module on top of #2670730: Provide a delete action for each content entity type, when I realized that media has no collection route.

The collection route is needed for the cancel URL in the media delete action.

It is also useful for creating links to the list at Content > Media (/admin/content/media).

Proposed resolution

Add a collection route to the media entity type.

After this patch, Url::fromRoute('entity.media.collection')->toString() can be used to generate the URL of /admin/content/media.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chr.fritsch created an issue. See original summary.

phenaproxima’s picture

Issue tags: +Media Initiative

Tagging.

chr.fritsch’s picture

Status: Active » Needs review
FileSize
6.36 KB

Here is a patch. The cool thing is, now we have a media list also without enabled views module.

chr.fritsch’s picture

marcoscano’s picture

Status: Needs review » Needs work

Nice work @chr.fritsch! Just some minors.

  1. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,138 @@
    +      'title' => $this->t('Title'),
    

    For the sake of consistency, in the view we call this "Media Name".

  2. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,138 @@
    +    $options += ($langcode != LanguageInterface::LANGCODE_NOT_SPECIFIED && isset($languages[$langcode]) ? ['language' => $languages[$langcode]] : []);
    

    Am I missing something or $languages hasn't been defined by this point?

  3. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,138 @@
    +    $row['status'] = $entity->isPublished() ? $this->t('published') : $this->t('not published');
    

    In the view we call these Published and Unpublished.

  4. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,138 @@
    +    $build = parent::render();
    

    For the record, the view by default shows older entities first, while this new collection shows older entities last. Do we want to make them the same? I don't have strong feelings about it, but wanted to mention it just in case.

  5. Also, do we want a test for this new collection?
chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
8.33 KB
3.82 KB

1. Fixed
2. Oh yes, it was copied from NodeListBuilder 🙈
3. Fixed
4. Fixed
5. Added a test for checking the collection route and if the media list is present

marcoscano’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

Sorry! Just a few minor things:

  1. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,154 @@
    +    if (\Drupal::languageManager()->isMultilingual()) {
    

    The language manager should probably be injected.

  2. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,154 @@
    +    $language_manager = \Drupal::languageManager();
    

    Same here.

  3. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,154 @@
    +    $options += ($langcode != LanguageInterface::LANGCODE_NOT_SPECIFIED && isset($languages[$langcode]) ? ['language' => $languages[$langcode]] : []);
    

    This is hard to parse. Can we refactor it into a more traditional if statement?

  4. +++ b/core/modules/media/tests/src/Functional/MediaUiFunctionalTest.php
    @@ -192,4 +192,32 @@ public function testRenderedEntityReferencedMedia() {
    +    $this->assertSession()->elementTextContains('css', 'table > thead > tr > th:nth-child(2)', 'Media name');
    +    $this->assertSession()->elementTextContains('css', 'table > thead > tr > th:nth-child(3)', 'Type');
    +    $this->assertSession()->elementTextContains('css', 'table > thead > tr > th:nth-child(7)', 'Operations');
    +
    +    // Media item is present.
    +    $this->assertSession()->elementTextContains('css', 'table > tbody > tr.odd > td:nth-child(2) > a', 'Unnamed');
    

    Can we call assertSession() once and reuse its return value?

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
9.24 KB
5.61 KB

Fixed everything from #8 and added a empty post update function, beacuse we are changing media.links.task.yml and media.links.action.yml.

Status: Needs review » Needs work

The last submitted patch, 9: 2934424-9.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review

Testbot hickup

phenaproxima’s picture

Status: Needs review » Needs work
+++ b/core/modules/media/media.post_update.php
@@ -0,0 +1,16 @@
+function media_post_update_collectio_route() {

Missing a final 'n' in the word 'collection'.

Other than that, looks great!

benjifisher’s picture

Status: Needs work » Needs review
FileSize
9.24 KB
369 bytes

I am just addressing the comment in #12. I have not reviewed nor tested myself.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I see nothing to complain about here!

benjifisher’s picture

Issue summary: View changes
seanB’s picture

+++ b/core/modules/media/media.post_update.php
@@ -0,0 +1,16 @@
+function media_post_update_collection_route() {
+  // Empty post-update hook.
+}

Is an empty post update hook the best way to clear the cache? Haven't seen this approach before.
Can't we clear it more specifically? I believe this whipes the entire cache with drupal_flush_all_caches().

chr.fritsch’s picture

I think core uses this method all the time to clear the cache. See system.post_update.php and it was suggested by @alexpott to do so.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2934424-13.patch, failed testing. View results

xjm’s picture

The fail in #13 was https://www.drupal.org/pift-ci-job/853314:

 Drupal\Tests\media\FunctionalJavascript\MediaUiJavascriptTest::testMediaTypes
Behat\Mink\Exception\ResponseTextException: The text "The media type pAkYI8C0 has been added." was not found anywhere in the text of the current page.

This could either be a random fail or regression introduced by a recent commit, or it could be something introduced by the patch here (due to changes in HEAD since #9).

We'll wait and see what the retest run says. :)

xjm’s picture

FWIW the fail in #19 looks like a classic "not waiting on the right UI element" JS test random fail.

benjifisher’s picture

I reported the intermittent failure on #2934997: Intermittent failure in MediaUiJavascriptTest.

xjm’s picture

Status: Needs work » Reviewed & tested by the community

Is an empty post update hook the best way to clear the cache? Haven't seen this approach before.

Yep, we do this all the time. Doing this ensures that the site owner runs update.php and that the cache is cleared once and only once at the end of the updates, rather than once for every update hook that needs a cache clear. :)

We've confirmed that #2934997: Intermittent failure in MediaUiJavascriptTest already happens in HEAD, so this issuer can go back to RTBC. (I have not reviewed it myself.)

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record
  1. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,167 @@
    +        '#style_name' => 'thumbnail',
    

    note to self and other reviewers, this is provided by image module, which is a hard dependency for media, so will always exist.

  2. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,167 @@
    +    if ($langcode != LanguageInterface::LANGCODE_NOT_SPECIFIED && isset($languages[$langcode])) {
    

    ubernit: we're comparing strings, we should use !==

  3. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,167 @@
    +    $build['table']['#empty'] = $this->t('No media items available. <a href=":url">Add media</a>.', [
    +      ':url' => Url::fromRoute('entity.media.add_page')->toString(),
    +    ]);
    

    should we be checking for 'create media' permission before doing this? i.e. adding a #access?

    Will need to be careful with cache tags if we add two different strings depending on access.

    Would require injecting the current user.

  4. +++ b/core/modules/media/tests/src/Functional/MediaUiFunctionalTest.php
    @@ -192,4 +192,34 @@ public function testRenderedEntityReferencedMedia() {
    +    $assert_session->elementTextContains('css', 'table > thead > tr > th:nth-child(2)', 'Media name');
    +    $assert_session->elementTextContains('css', 'table > thead > tr > th:nth-child(3)', 'Type');
    +    $assert_session->elementTextContains('css', 'table > thead > tr > th:nth-child(7)', 'Operations');
    ...
    +    $assert_session->elementTextContains('css', 'table > tbody > tr.odd > td:nth-child(2) > a', 'Unnamed');
    

    Are we sure we want to be this specific with the column indexes, it makes the test more fragile.

    If we care about column order, leave it as is.

    If we want to test that the column exists, but don't care about order, then a less fragile approach might be:

    $assert->elementExists('th:contains("Media name")');
    

    And so on.

I think we also need a change record here, not for the list builder, but for the presence of the collection route.

chr.fritsch’s picture

Thanks for reviewing @larowlan

I addressed all the feedback form #23

2. Fixed
3. I removed the complete function override and let EntityListBuilder do the stuff like we do it in NodeListBuilder. EntityListBuilder already has a @todo to add the "Add"-link.
4. Fixed

tstoeckler’s picture

Re #23.1: While the config is initially created by Image module, it's entirely possible to delete the default thumbnail image style. As far as I can tell this would then lead to a fatal error on the new media overview provided here (i.e. if Views module is uninstalled), so I actually do think it would make sense to have some sort of fallback.

chr.fritsch’s picture

I discussed #25 with @marcoscano and we agreed that just checking it the thumbnail style exists should be fine.

chr.fritsch’s picture

tstoeckler’s picture

The interdiff in #26 looks good to me, thanks, but leaving to @larowlan for feedback and hopefully re-RTBC ;-)

tstoeckler’s picture

Status: Needs review » Needs work

So in order to push this forward I wanted to RTBC myself, so that perhaps @larowlan can commit. In reading through the patch in detail and trying it out I did find a couple of minor things, though. Mostly nitpicks, but all in all enough to push this back just one more time.

  1. +++ b/core/modules/media/media.links.task.yml
    @@ -23,3 +23,9 @@ entity.media_type.collection:
    +  weight: 1
    

    It's nice to give contrib/custom modules some "space" in case they want to come in between this and whatever is at weight 0, so I would suggest 10. On the other hand, is it actually wanted behavior that this has a higher weight than the other links? Neither "Comments" and "Files" which are displayed at the same spot set a weight, so they are just ordered alphabetically. Is there are a reason we are not doing the same here?

  2. +++ b/core/modules/media/media.post_update.php
    @@ -0,0 +1,16 @@
    + * Cache clear.
    + *
    + * Clear caches due to changes in media.links.task.yml and
    + * media.links.action.yml.
    

    The description of (post-)update functions is actually shown to the user on /update.php, so we should only use a single line and not a long description. Also it should be a proper sentence describing what the function does, i.e. "Clears caches." instead of "Cache clear.". We could also be more explicit, i.e. "Clear caches due to changes in local tasks and action links." or something like that, but not sure if that's better or worse than the short version.

  3. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,171 @@
    +   * Indicates if the 'thumbnail' image style exists.
    

    I think "if" -> "whether"

  4. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,171 @@
    +    $this->thumbnailStyleExists = !empty($image_style_storage->load('thumbnail'));
    

    This is arguable, so feel free to ignore, but alternatively you could just cast to (bool) instead of calling empty().

  5. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,171 @@
    +      $container->get('entity.manager')->getStorage($entity_type->id()),
    ...
    +      $container->get('entity.manager')->getStorage('image_style')
    

    Could use entity_type.manager instead of entity.manager

  6. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,171 @@
    +    // Enable language column and filter if multiple languages are added.
    

    I don't actually see a filter.

  7. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,171 @@
    +      $header['language_name'] = [
    ...
    +      $row['language_name'] = $this->languageManager->getLanguageName($langcode);
    

    This is super nitpicky but just language would be more consistent with e.g. author.

  8. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,171 @@
    +    $options = $uri->getOptions();
    +    if ($langcode !== LanguageInterface::LANGCODE_NOT_SPECIFIED && isset($languages[$langcode])) {
    +      $options += ['language' => $languages[$langcode]];
    +    }
    +    $uri->setOptions($options);
    

    This is actually not necessary as $entity->toUrl() will already add the language option.

  9. +++ b/core/modules/media/src/MediaListBuilder.php
    @@ -0,0 +1,171 @@
    +    $row['operations']['data'] = $this->buildOperations($entity);
    +    return $row + parent::buildRow($entity);
    

    The operations are already added by the parent so it should not be necessary to add them explicitly. That also matches ::buildHeader() where we do not add the operations explicitly either.

  10. +++ b/core/modules/media/tests/src/Functional/MediaUiFunctionalTest.php
    @@ -192,4 +192,33 @@ public function testRenderedEntityReferencedMedia() {
    +    /** @var \Drupal\Core\Entity\Sql\SqlContentEntityStorage $media_storage */
    

    Super nitpick: Could just as well use EntityStorageInterface as nothing specific from SqlContentEntityStorage is needed here.

  11. +++ b/core/modules/media/tests/src/Functional/MediaUiFunctionalTest.php
    @@ -192,4 +192,33 @@ public function testRenderedEntityReferencedMedia() {
    +    $media_storage = $this->container->get('entity.manager')->getStorage('media');
    

    Could use entity_type.manager instead of entity.manager.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
8.95 KB
4.31 KB

Addressing #29

1. I gave it a weight of 10. Without any weight, it would be placed between "Comments" and "Files". Let's keep the current order.
2. + 3. Fixed
4. Ignored 😏
5. + 6. Fixed
7. Fixed
8. Awesome 👍
9. Fixed
10. + 11. Fixed

Thanks for reviewing, @tstoeckler!

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Awesome, thanks a lot! Looks beautiful now. I tried hard but couldn't find anything else to complain about ;-)

Also there is already a change notice, so removing that tag.

larowlan’s picture

Adding review credits for those that helped shape the final patch.

Crediting @xjm for mentoring (in person and online) in identifying the random fail in HEAD.

  • larowlan committed ba4f89e on 8.5.x
    Issue #2934424 by chr.fritsch, benjifisher, tstoeckler, phenaproxima,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

fixed on commit

diff --git a/core/modules/media/src/MediaListBuilder.php b/core/modules/media/src/MediaListBuilder.php
index d4eed71..a0f1f02 100644
--- a/core/modules/media/src/MediaListBuilder.php
+++ b/core/modules/media/src/MediaListBuilder.php
@@ -7,9 +7,7 @@
 use Drupal\Core\Entity\EntityListBuilder;
 use Drupal\Core\Entity\EntityStorageInterface;
 use Drupal\Core\Entity\EntityTypeInterface;
-use Drupal\Core\Language\LanguageInterface;
 use Drupal\Core\Language\LanguageManagerInterface;
-use Drupal\Core\Url;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**

Committed as ba4f89e and pushed to 8.5.x

Published the change record

Status: Fixed » Closed (fixed)

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

lakshmanDrupal’s picture

FileSize
4.07 KB

I was able to see this on Drupal 9.5