Skip to content
Snippets Groups Projects

Issue #2971209: Allow the MediaLibraryUiBuilder service to use an alternative view display

Open Issue #2971209: Allow the MediaLibraryUiBuilder service to use an alternative view display
3 unresolved threads
3 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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:

    1. 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.
    2. MediaLibraryState should 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.
    3. MediaLibraryState should provide a getter for the view and display, just like it has getters for getOpenerId(), 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
  • Separately, the parameter bag parameters should be made consistent (either view_id and view_display_id; or views_id and views_display_id).

  • Guy Elsmore-Paddock changed this line in version 2 of the diff

    changed this line in version 2 of the diff

  • Please register or sign in to reply
  • Guy Elsmore-Paddock added 1424 commits

    added 1424 commits

    Compare with previous version

  • added 1 commit

    • a2d93733 - Issue #2971209 - Re-roll of Comment 27 but without Admin Configurability

    Compare with previous version

  • 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.
    • I think adding separate arguments for the view name and display ID would be clearer and helps us so we don't have to explode strings and do the dot validation.

    • 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.

      image

      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
    • John Karahalis changed this line in version 6 of the diff

      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
    • Please register or sign in to reply
  • 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.

    • Please register or sign in to reply
  • Guy Elsmore-Paddock added 958 commits

    added 958 commits

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Guy Elsmore-Paddock mentioned in merge request !4019

    mentioned in merge request !4019

  • John Karahalis added 5 commits

    added 5 commits

    Compare with previous version

  • John Karahalis added 1 commit

    added 1 commit

    Compare with previous version

  • John Karahalis added 1 commit

    added 1 commit

    Compare with previous version

  • John Karahalis added 1 commit

    added 1 commit

    Compare with previous version

  • John Karahalis added 1 commit

    added 1 commit

    Compare with previous version

  • Please register or sign in to reply
    Loading