Media Library Add entity-type form_mode config from widget

Fixing patch issue

Fixing CCF #2

1. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/MediaLibrary.php
   @@ -81,10 +81,14 @@ public function getDynamicPluginConfig(array $static_plugin_config, EditorInterf
   +    // @todo where should this be pulled from? The field widget specifies.
   

   this would have to be something set on the media-embed filter plugin config and we get it from there.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -117,7 +118,9 @@ protected function getMediaType(FormStateInterface $form_state) {
   +    $this->mediaType = $this->entityTypeManager
   

   please revert this whitespace change.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -152,6 +161,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      Html::getClass('media_library_form_mode--' . $add_form_mode),
   

   no need to add a css class here.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -191,7 +201,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -        $form['media'][$delta] = $this->buildEntityFormElement($media, $form, $form_state, $delta);
   

   Can we add the form mode as a stored value in form_state? I see that as a more normal mechanism to pass around data for forms.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -191,7 +201,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        $form['media'][$delta] = $this->buildEntityFormElement(
   +          $media, $form, $form_state, $delta, $add_form_mode);
   

   please don't change the whitespace here.

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -245,11 +256,15 @@ abstract protected function buildInputElement(array $form, FormStateInterface $f
   +  protected function buildEntityFormElement(MediaInterface $media, array $form,
   +    FormStateInterface $form_state, int $delta, string $add_form_mode = 'media_library') {
   

   This isn't needed if we pass the form mode as a stored value in form state.

7. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -257,13 +257,24 @@ public function processUploadElement(array $element, FormStateInterface $form_st
   +   * Build the sub form for media entities, but modify base to slim it down.
   

   Please revert this change.

8. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -257,13 +257,24 @@ public function processUploadElement(array $element, FormStateInterface $form_st
   +      $element['fields'][$source_field]['widget'][0]['#process'][] = [
   

   please revert this whitespace change.

9. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -271,6 +282,9 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +   * Removes the remove_button, preview, title, description and filename
   

   please revert this change.

10. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -358,6 +372,7 @@ protected function createFileItem(MediaTypeInterface $media_type) {
    +    // @todo $media->bundle is not part of MediaInterface.
    

    please revert this.

11. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -381,6 +396,7 @@ public function removeButtonSubmit(array $form, FormStateInterface $form_state)
    +    // @todo $media->bundle is not part of MediaInterface.
    

    please revert this.

12. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -71,11 +81,12 @@ public function __construct(array $parameters = []) {
    +  public static function create($opener_id, array $allowed_media_type_ids, $selected_type_id, $form_mode, $remaining_slots, array $opener_parameters = []) {
    
    +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -262,14 +286,20 @@ public static function setMediaTypesValue(array &$element, $input, FormStateInte
    +
    

    This breaks a lot of contrib modules when we add the form mode parameter. We'll need to add it as the last parameter and do a BC dance.

13. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -239,7 +239,14 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
    -      $link_state = MediaLibraryState::create($state->getOpenerId(), $state->getAllowedTypeIds(), $allowed_type_id, $state->getAvailableSlots(), $state->getOpenerParameters());
    +      $link_state = MediaLibraryState::create(
    

    please revert most of the whitespace changes here.

14. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -310,7 +317,9 @@ protected function buildMediaTypeAddForm(MediaLibraryState $state) {
    -    return $this->formBuilder->buildForm($plugin_definition['forms']['media_library_add'], $form_state);
    +
    +    $form_class = $plugin_definition['forms']['media_library_add'];
    +    return $this->formBuilder->buildForm($form_class, $form_state);
    

    this change isn't needed. can we revert it.

15. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -217,6 +230,17 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +    $form_modes = $this->entityDisplayRepository
    +      ->getFormModeOptions('media');
    
    @@ -262,14 +286,20 @@ public static function setMediaTypesValue(array &$element, $input, FormStateInte
    +    $form_modes = $this->entityDisplayRepository->getFormModeOptions('media');
    

    In the first case, we line wrap. In the second, we do it one longer line. I prefer the second as its less connotative dissonance to read across 2 lines when the things are fairly short. Let's do the same thing in both places.

16. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -262,14 +286,20 @@ public static function setMediaTypesValue(array &$element, $input, FormStateInte
    -    $media_type_labels = [];
    ...
    +      $media_type_labels = [];
    

    If we don't remove this line and move it down below, it makes the code more happy when applying in 9.4/9.5. Let's not fix up anything except the summary additions.

17. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -262,14 +286,20 @@ public static function setMediaTypesValue(array &$element, $input, FormStateInte
    +
    

    Whitespace change. Please revert.

18. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -496,7 +526,17 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +    // @todo where should this be pulled from? The field widget specifies.
    

    The field widget should have this as a config option.

19. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -531,7 +571,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    -      if ($triggering_element && ($trigger_parents = $triggering_element['#array_parents']) && end($trigger_parents) === 'media_library_update_widget') {
    

    whitespace noise. please revert.

20. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetAccessTest.php
    @@ -58,14 +58,23 @@ public function testWidgetAccess() {
    +    $form_mode = 'media_library';
    

    this is a test, no need for a todo here.

21. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetAccessTest.php
    @@ -58,14 +58,23 @@ public function testWidgetAccess() {
    +    $state = MediaLibraryState::create(
    

    please revert the whitespace fixes here.

22. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetAccessTest.php
    @@ -125,6 +134,9 @@ public function testWidgetAccess() {
    +      'query' => array_merge($url_options['query'], ['media_library_form_mode' => '   ']),
    

    is the form mode intentionally empty here?

23. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -108,11 +108,20 @@ public function testFieldWidgetEntityCreateAccess() {
    +    $state = MediaLibraryState::create(
    

    please revert the whitespace changes here.

24. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -158,10 +167,14 @@ public function testEditorOpenerAccess($media_embed_enabled, $can_use_format) {
    +    // @todo where should this be pulled from? The field widget specifies.
    

    no need to ask. this is a test.

25. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -213,13 +226,23 @@ public function testFieldWidgetEntityEditAccess() {
    +    $state = MediaLibraryState::create(
    

    please revert whitespace.

26. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -302,8 +326,17 @@ public function testFieldWidgetEntityFieldAccess(string $field_type) {
    +    // @todo where should this be pulled from? The field widget specifies.
    

    no need to ask, this is a test.

27. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -302,8 +326,17 @@ public function testFieldWidgetEntityFieldAccess(string $field_type) {
    +    $state = MediaLibraryState::create(
    

    please revert whitespace.

28. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -339,11 +373,20 @@ public function testViewAccess() {
    +    // @todo where should this be pulled from? The field widget specifies.
    

    same here. and through the rest of the tests. for both the form mode and the whitespace changes.

Here's some clean-up. I'll have some comments coming next.

1. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -41,14 +43,25 @@
   +  public const DEFAULT_FORM_MODE = 'media_library';
   
   +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -45,6 +46,11 @@
   +  const DEFAULT_VIEW_MODE = 'media_library';
   

   These are duplicates of each other. Let's use the value from State everywhere instead of the widget.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetAccessTest.php
   @@ -125,6 +125,9 @@ public function testWidgetAccess() {
   +    $this->drupalGet('media-library', [
   

   What is this testing?

3. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAddFormTest.php
   @@ -35,6 +35,13 @@ class MediaLibraryAddFormTest extends KernelTestBase {
   +  protected $form_mode = 'media_library';
   
   @@ -112,7 +119,7 @@ public function testMediaTypeAddForm() {
   +    $state = MediaLibraryState::create('test', ['image', 'remote_video'], $selected_type_id, -1, [], $this->form_mode);
   

   Let's use the constant from State instead of this value here. Let's use the constants in all the creates directly instead of a class level property.

4. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryStateTest.php
   @@ -21,6 +21,11 @@ class MediaLibraryStateTest extends KernelTestBase {
   +  protected $form_mode = MediaLibraryState::DEFAULT_FORM_MODE;
   
   @@ -70,14 +75,14 @@ public function testMethods() {
   +    $state = MediaLibraryState::create($opener_id, $allowed_media_type_ids, $selected_media_type_id, $remaining_slots, [], $this->form_mode);
   

   Instead of setting this as a class level property, let's do individually on each of the state constructions. More configurablity and it will be clearer what is going on.

1. If we aren't touching view mode, then we shouldn't touch it at all. Let's keep out of scope changes to a minimum.
2. Hmm, there was already an earlier invalid value, right? How would this do anything that wasn't already happening?
3. If we need to use a wrong value, by all means don't use the constant. But if we are using the right value, then we should use the constant.

Why didn't this build on top of #18? Or at least it doesn't seem like it builds?

Fixed CCF for #26.
Please review.

+++ b/core/modules/media_library/config/schema/media_library.schema.yml
@@ -8,6 +8,9 @@ field.widget.settings.media_library_widget:
+    form_mode:

we'll need an update hook to add a default value for the form_mode anywhere the widget is used.

This will also need a CR at some point. Several contrib modules have the form mode hard coded. For example, focal_point uses a hard-coded value.

Styling of links got broken after some recent core update. Going for a local solution and documenting here in case this issue got more attention again eventually:
in implementation of hook_preprocess_links__media_library_menu()

$variables['#attached']['library'][] = 'core/drupal.vertical-tabs';

Probably a vertical tabs #type should be used here instead of the '#theme' => 'links__media_library_menu', that has a couple of preprocess in various themes but not much, currently basically those are links and not much more.

Ok, actually it's a core issue and has nothing to do with the work here. Reproduced on a clean instance with no patch.
Ignore my last comment please.

I started an MR targeting 11.x with a rebased version of 3323232-29.patch.

Here's a re-roll for 10.3 with test changes removed for more simplicity.

No comment.