Issue #2971209: Allow the MediaLibraryUiBuilder service to use an alternative view display
Merge request reports
Activity
346 344 return $view_executable->buildRenderable($display_id, $args, FALSE); 347 345 } 348 346 347 /** 348 * Returns the ID of the media library view. 349 * 350 * @param \Drupal\media_library\MediaLibraryState $state 351 * The current state of the media library, derived from the current request. 352 * 353 * @return string 354 * The ID of the media library view. 355 */ 356 protected function getViewId(MediaLibraryState $state): string { 357 return $state->get('view_id', 'media_library'); - Comment on lines +357 to +361
Some recommendations:
- The name of the default view ID and name of the default display ID (on line 324/326) really should be concerns of the
MediaLibraryState, not the UI builder. The UI builder is a consumer of the state but should not be concerned with how the values in the state got there. -
MediaLibraryStateshould provide a way for this view and display to be populated at construction time, just like all other creation-time parameters, so that it's not awkward to set the view and display. If the concern is that the state is already taking in a lot of parameters, perhaps a new, final variadic/spread parameter should be added to the constructor that allows it to take in additional parameters that can be added to the parameter bag. -
MediaLibraryStateshould provide a getter for the view and display, just like it has getters forgetOpenerId(),getSelectedTypeId(), etc. That frees consumers up from having to hard-code the name of the parameter bag keys for the view and display.
Presently--if 2 above is not addressed and a caller wants to provide the view and display--the pattern looks like this:
$library_state = MediaLibraryState::create( 'media_library.opener.field_widget', $allowed_media_type_ids, $initial_media_type_id, $remaining_count, $opener_parameters ); $library_state->set('view_id', $view_id); $library_state->set('views_display_id', $display_id);... which is a bit awkward.
What I am proposing is something like this:
$library_state = MediaLibraryState::create( 'media_library.opener.field_widget', $allowed_media_type_ids, $initial_media_type_id, $remaining_count, $opener_parameters, view_id: $view_id, views_display_id: $display_id );But with the
create()method declared as follows (using a named variadic), so that the view and display don't need to be explicit parameters:class MediaLibraryState extends ParameterBag implements CacheableDependencyInterface { // ... public static function create($opener_id, array $allowed_media_type_ids, $selected_type_id, $remaining_slots, array $opener_parameters = [], ...$additional_state_parameters) { // ... } // ... }Edited by Guy Elsmore-Paddock - The name of the default view ID and name of the default display ID (on line 324/326) really should be concerns of the
changed this line in version 2 of the diff
added 1424 commits
-
da63f415...bfbf08c0 - 1422 commits from branch
project:9.3.x - 0cd660fc - Add getViewId() method.
- 0e6932cc - Issue #2971209 - Re-roll of #27 but without Admin Configurability
-
da63f415...bfbf08c0 - 1422 commits from branch
added 1 commit
31 32 * items that can be selected, it can pass the number of remaining slots. When 32 33 * the number of remaining slots is a negative number, an unlimited amount of 33 34 * items can be selected. 35 * - media_library_view_display: The view and display to use in the media library. -
Agreed.
The convention of dot-delimiting the view and display as a single quantity came from #27 because that added a drop-down selector in the widget settings where you could pick the display that the library should use.
Even though the latest patch removes the site builder UI, a contrib. maintainer (like my team) may want to expose their own UI for selecting the view and display to use. In such a case, I don't believe we want to split the selector out to two separate drop-down widgets -- one for views and one for displays -- because then we have to concern ourselves with only providing the site builder with displays that match the view they have selected.
At the same time, we can certainly abstract the way the admin UI needs to display this from the way that the state object stores it. So, we could still dot-delimit the value for the admin form but then store the values separately.
Edited by Guy Elsmore-Paddock changed this line in version 6 of the diff
This has been taken care of in !4019, the superseding merge request.
Edited by John Karahalis
166 186 if (!is_numeric($remaining_slots)) { 167 187 throw new \InvalidArgumentException('The remaining slots parameter is required and must be numeric.'); 168 188 } 189 190 // The view display must be dot-delimited and reference a valid view and display. 191 if (strpos($view_display, '.') === FALSE) { 192 throw new \InvalidArgumentException('The view ID and display ID must be joined by a period (".").'); 193 } 194 [$view_id, $display_id] = explode('.', $view_display); 195 $view_storage = \Drupal::entityTypeManager()->getStorage('view'); -
I'm not really sure if we should depend on the entity type manager in this value object. I guess we already added a service dependency for the private key and hashing, but we might want to keep the state as simple as possible. It was supposed to be used as a simple value object to pass values around. We also don't really check if the media types passed to the state actually exists.
-
@seanpenn079 Although I agree that it is not desirable to add yet another dependency to what should be a simple data object, I don't see a way around it unless we move all the validation responsibilities of this class to a separate factory service like we discussed in the thread.
Though it is true that we do not check if the media types actually exist, if one of the allowed media types is missing the view could still render. If the display or view are missing -- on the other hand -- we can't render any UI back to the user and we will most likely render a cryptic error to the user rather than a very specific error that the view or display are missing. Consider that one of the places where the media library state object is constructed is from the request (via the
fromRequest()method). It's therefore important to validate that the view and display are valid because it is possible that an opener could load and generate a URL that references a view and display that exist at the time the page loads that do not exist when the media library is being opened.For consistency, I'd expect either that the media library state object would validate all the values it's being passed, or none of them -- rather than picking and choosing which state we care about and which we do not. If we don't want this object handling the validation, then we should move that responsibility over to a factory object and make other classes work through that factory to construct instances of the state object. The factory, then, can have any dependencies necessary for that validation while keeping the state object simple, as you have requested.
added 958 commits
- a2d93733...178f6204 - 948 earlier commits
- 260b4d16 - Drupal 9.4.14
- 0cc93453 - Merged 9.4.14.
- 05ea37f0 - Back to dev.
- 72e3bf85 - Issue #3357247 by Spokje: Update guzzlehttp/psr7
- b5688b23 - Issue #3356283 by longwave, szato, catch, Dave Reid, Spokje, effulgentsia,...
- bb57559c - Issue #3357881 by longwave, alexpott: Update composer.lock hash for 9.4.x
- b199420c - Drupal 9.4.15
- a38428ec - Add getViewId() method.
- 0d982463 - Issue #2971209 - Re-roll of Comment 27 but without Admin Configurability
- 8080c3f4 - Issue #2971209 - Remove @todo Related to This Issue
Toggle commit listadded 1 commit
mentioned in merge request !4019
added 5 commits
Toggle commit listadded 1 commit
added 1 commit
added 1 commit
added 1 commit
mentioned in commit inline_media_form@31e2e12d