Problem/Motivation
This got spun off from #2993187-34: Add a media library widget setting to only choose existing media and subsequent discussion. The media library already delegates access control for the "add new media" form to the entity system, but it does not provide any additional context (e.g., the MediaLibraryState
object). If it did, then contrib modules and custom code could support more complex access control, including the functionality requested in #2993187: Add a media library widget setting to only choose existing media.
Steps to reproduce
N/A
Proposed resolution
In \Drupal\media_library\MediaLibraryUiBuilder::buildMediaTypeAddForm()
, pass the MediaLibraryState
object to the access control handler, in its $context
parameter.
Remaining tasks
Create a merge request with test coverage.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
TBD.
Issue fork drupal-3224530
Show commands
Start 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:
Comments
Comment #3
phenaproximaTransferred the code from the merge request in #2993187: Add a media library widget setting to only choose existing media.
Comment #4
seanBThat looks good to me! Thanks.
Comment #5
phenaproximaTagging this as a contrib project blocker, since it's needed in order to implement #3224564: Add a media library field widget setting to hide the "add media" form.
Comment #6
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think we should add something to the test that makes the access decision based on something that's in the library state (not just that some library state got passed). That would also help clarify what an example use-case for it is.
Comment #7
phenaproximaGood call, @effulgentsia. I changed it so that the hook actually tries to use the state object to make an access decision.
Comment #8
seanBComment #9
larowlanCouple of questions on the MR, looks good though, nice and simple
Comment #10
phenaproximaI take it back. If you look at the code of MediaLibraryUiBuilder, you'll see that it
$build['content']
always has a 'form' element (see thebuildLibraryContent()
method). But it is always going to be absolutely empty if access is denied, per this code at the top ofbuildMediaTypeAddForm()
:So I think that, to prevent further refactoring and scope creep, assertEmpty() and assertNotEmpty() are the correct assertions for us to use.
Comment #11
phenaproximaReplied to all threads; for various reasons, I'm not sure any changes are needed here. So, tentatively restoring RTBC.
Comment #12
larowlanOk, thanks for entertaining my questions.
As there are no changes since my review, restoring @seanB's RTBC
Comment #13
larowlanCan we get a change record here detailing that this is available in $context
Comment #14
phenaproximaCreated https://www.drupal.org/node/3225885 to document the change.
Comment #15
larowlanComment #17
larowlanCommitted 16edf0a and pushed to 9.3.x. Thanks!
Published the change record