Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
.
Comment | File | Size | Author |
---|---|---|---|
#36 | media_issues.PATCH | 4.07 KB | lakshmanDrupal |
#30 | interdiff-2934424-26-30.txt | 4.31 KB | chr.fritsch |
#30 | 2934424-30.patch | 8.95 KB | chr.fritsch |
#26 | interdiff-2934424-24-26.txt | 3.51 KB | chr.fritsch |
#26 | 2934424-26.patch | 9.4 KB | chr.fritsch |
Comments
Comment #2
phenaproximaTagging.
Comment #3
chr.fritschHere is a patch. The cool thing is, now we have a media list also without enabled views module.
Comment #4
chr.fritschIn #2932369: Media Types missing access control handler result in empty column in media overview page we are fixing the media type label. So we should do this here as well.
Comment #5
marcoscanoNice work @chr.fritsch! Just some minors.
For the sake of consistency, in the view we call this "Media Name".
Am I missing something or
$languages
hasn't been defined by this point?In the view we call these
Published
andUnpublished
.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.
Comment #6
chr.fritsch1. 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
Comment #7
marcoscanoLooks good to me
Comment #8
phenaproximaSorry! Just a few minor things:
The language manager should probably be injected.
Same here.
This is hard to parse. Can we refactor it into a more traditional if statement?
Can we call assertSession() once and reuse its return value?
Comment #9
chr.fritschFixed everything from #8 and added a empty post update function, beacuse we are changing media.links.task.yml and media.links.action.yml.
Comment #11
chr.fritschTestbot hickup
Comment #12
phenaproximaMissing a final 'n' in the word 'collection'.
Other than that, looks great!
Comment #13
benjifisherI am just addressing the comment in #12. I have not reviewed nor tested myself.
Comment #14
phenaproximaI see nothing to complain about here!
Comment #15
benjifisherComment #16
seanBIs 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()
.Comment #17
chr.fritschI 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.
Comment #19
xjmThe fail in #13 was https://www.drupal.org/pift-ci-job/853314:
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. :)
Comment #20
xjmFWIW the fail in #19 looks like a classic "not waiting on the right UI element" JS test random fail.
Comment #21
benjifisherI reported the intermittent failure on #2934997: Intermittent failure in MediaUiJavascriptTest.
Comment #22
xjmYep, 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.)
Comment #23
larowlannote to self and other reviewers, this is provided by image module, which is a hard dependency for media, so will always exist.
ubernit: we're comparing strings, we should use !==
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.
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:
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.
Comment #24
chr.fritschThanks 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
Comment #25
tstoecklerRe #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.Comment #26
chr.fritschI discussed #25 with @marcoscano and we agreed that just checking it the thumbnail style exists should be fine.
Comment #27
chr.fritschHere is the CR: https://www.drupal.org/node/2935083
Comment #28
tstoecklerThe interdiff in #26 looks good to me, thanks, but leaving to @larowlan for feedback and hopefully re-RTBC ;-)
Comment #29
tstoecklerSo 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.
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?
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.
I think "if" -> "whether"
This is arguable, so feel free to ignore, but alternatively you could just cast to
(bool)
instead of callingempty()
.Could use
entity_type.manager
instead ofentity.manager
I don't actually see a filter.
This is super nitpicky but just
language
would be more consistent with e.g.author
.This is actually not necessary as
$entity->toUrl()
will already add the language option.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.Super nitpick: Could just as well use
EntityStorageInterface
as nothing specific fromSqlContentEntityStorage
is needed here.Could use
entity_type.manager
instead ofentity.manager
.Comment #30
chr.fritschAddressing #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!
Comment #31
tstoecklerAwesome, 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.
Comment #32
larowlanAdding 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.
Comment #34
larowlanfixed on commit
Committed as ba4f89e and pushed to 8.5.x
Published the change record
Comment #36
lakshmanDrupal CreditAttribution: lakshmanDrupal at Un.titled commentedI was able to see this on Drupal 9.5