If you attempt to add contextual filters to the core media library, they will correctly be used when the library results are initially loaded, but are not passed to the view itself, so internal display links (the "grid" and "table" buttons) get URLs built that ignore the contextual arguments.
Add/edit the any content type
Add media reference field, in manage form display of content type: select
'media library' display
Then edit the view of core media libray (widget display):
https://your_site_url/admin/structure/views/view/media_library
Then add a contextual argument to view displays (for eg. wigets , widget table), such as "media: authored by" and select "user ID from logged in user" as the default value or either can add fixed uid for test purpose
Add node, try to add media , Click on one of the display buttons in the header, either grid or table, you 'll find contextual filter not work as view will show all media instead of just your own.
Click on one of the display buttons in the header, either grid or table, and the view will refresh to show all media instead of just your own. This is wrong.
when try to upload image using a media library widget, then view that is loaded only shows the media your user has created. This is correct.
Replace the current build call in buildMediaLibraryView()
return $view_executable->buildRenderable($display_id, $args, FALSE);
with
return $view_executable->buildRenderable($display_id, [], FALSE);
I did initially try this with $view_executable->args, but that seems to be redundant and passing [] has exactly the same effect. Current behaviour which passes through just the entity type argument ($args) stops any other contextual filters from working securely when the view is rendered. The $view_executable->execute($args) call a few lines earlier already does the full argument calculations for all contextual filters.
Add tests
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
dieuweComment #3
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
Comment #7
simePatch worked for me, and I've created the issue fork for it.
Comment #8
manthan.chauhan commentedI confirm that this patch fixes the issue since i was having the same issue.
Comment #9
erwangel commentedConfirmed working by me too (on D10.2.6).
Comment #10
simeRebased and tests passing. There was an failed test that passwed when i reran.
Comment #11
quietone commentedI read the issue summary, comments and the MR. I do think that there should be a test here.
I also tried to test this but wasn't able to reproduce the problem. Following the steps to reproduce I found that the 'Grid' showed only media from the logged in user and the 'Table' showed media for all users. This was true with or without the diff applied. But, I seem to be in the minority here about that.
Still, I do recommend tests.
Comment #12
herved commentedAmazing!
I debugged this for a while before commenting #3142777: Media Library view widgets do not support contextual filters and implementing a service decorator to modify $args...
This is a much nicer fix and works perfectly in my case as well.
I'll see if I can add tests on Monday.
Thanks
Comment #14
pavlosdanAdded test. Setting back to needs review.
Comment #15
pooja_sharma commentedI have updated IS w.r.t to standard issue template along with added details steps for better clarity, as remaining task - Add test addressed , move it from the remaining tasks section to the proposed resolution.
Comment #16
pooja_sharma commentedI have applied some changes along with perms one those 'll improve performance & optimize the resource usability of test code.
Keeping it in review for further suggestions.
Comment #17
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #18
pooja_sharma commentedComment #19
herved commentedIf my understanding is correct, any AJAX request inside the modal was causing the form ID to change resulting in an HTML response instead of JSON which JS can't process and we get AJAX "parsererror". Some more details here: #3142777-22: Media Library view widgets do not support contextual filters
A quick way I used to debug was to click "Apply filters", then "Insert selected" -> Uncaught Drupal.AjaxError in console
Should we add something these steps to the test for good measure?
Also, couldn't we create the views handler programmatically, to simplify and save some time.
PS: I tried to push some those changes but it seems I don't have access. Anyway I wouldn't want to interfere.
PS2: Right, I had to click on "get push access", let me know if you agree with the changes.
Besides that it looks great to me!
And I think we can close #3142777: Media Library view widgets do not support contextual filters as duplicate.
Comment #20
pooja_sharma commentedI have reviewed the recent changes looks good in test code adding views handler programmatically instead of making http request improved the resource usage from Time: 00:22.841, Memory: 6.00 MB to Time: 00:16.287, Memory: 6.00 MB
Keeping it in review for further suggestions.
Comment #21
smustgrave commentedSeems the tests were skipped vs failing so wonder if they need some work.
Comment #22
herved commented#21, I had the same in this issue: #3464340: JS errors from Drupal.behaviors.activeLinks (... is not a valid selector)
Is the "Test-only changes" pipeline working correctly? Is it skipping FunctionalJavaScript tests for some reason?
I'm 99% sure tests in both issues fail as expected...
Comment #23
pavlosdanAsked in the #gitlab channel on Drupal slack and there was indeed a problem with the pipeline. See: #3467080: Test-only job cannot be run due to wrong dependency
It is now resolved. Rebased the fork and the tests seem to be triggering now: https://git.drupalcode.org/issue/drupal-3401726/-/pipelines/249631
Comment #25
tobiasbRebased. Test-Only fails.
But I will also test the fix in a real use-case.
Comment #26
tobiasbComment #27
smustgrave commentedTest-only ran here https://git.drupalcode.org/issue/drupal-3401726/-/jobs/7322548
Rebased as it was several commits behind but still green and believe all feedback has been resolved.
Comment #28
longwaveBackported to 11.3.x as an eligible bug fix. This is also eligible to go to 10.6.x but the tests will need reworking for the older version of PHPUnit, marking patch to be ported - to be honest given the one line fix I think we could just backport the fix here and be done, so if anyone wants to open a one-line MR for that then please do so.
Committed and pushed 3c10d8c26ce to main and aca36058964 to 11.x and 4f49a0d113a to 11.3.x. Thanks!
Comment #32
longwaveComment #35
smustgrave commentedBackported to 10.6.x
Comment #38
godotislateCommitted 4238966 and pushed to 10.6.x. Thanks!