Live preview is broken when editing the media_library view

working on it.

Couldn't quite detect if the view is in preview on that hook... Tried with something like empty($view->live_preview), which seems to be used in other parts of core, but it doesn't work, not sure why. In any case, if we see there is no opener ID for instance, there is no point in trying to continue processing the parameters.

Somehow I managed to mess up with the diff for the test-only patch above.
Here are hopefully the correct files (there is no difference on the actual patch from #3, that's why I'm keeping their filenames).

This is a better implementation, but same idea.

One thing I'm pondering here is whether or not this will result in hiding errors for users.

+++ b/core/modules/media_library/media_library.module
@@ -70,7 +70,15 @@ function media_library_views_post_render(ViewExecutable $view, &$output, CachePl
+      catch (InvalidArgumentException $e) {
+        return;
+      }

Should we re-throw the exception if $view->preview is FALSE?

+++ b/core/modules/media_library/media_library.module
@@ -70,7 +70,15 @@ function media_library_views_post_render(ViewExecutable $view, &$output, CachePl
+        return;

I wondered about whether we should remove the early return here. It's not necessary as we do if (!empty($query)) { later. But we can do that later if in future media_library_views_post_render does more.

After discussing more with @phenaproxima we worked out that we have a couple of issues here.

1. The media library view is not previewable at all so this issue is a band-aid - we should have a follow-up that implements something to allow view displays to declare that they can not be previewed.
2. This code should be a work around with an @todo to that issue and should only eat exceptions if (!empty($view->preview)) - both ->live_preview and ->preview are TRUE at this point. It'd be good to also file a follow-up to deprecate live_preview or to properly discern the difference between the two.
3. We should probably emit a message telling the user the view is not previewable here because they probably expect it to be previewable. Ideally instead of the drupal message this would be outputted in the preview area.

I agree we should only catch the exception when in preview mode.

Regarding whether or not the view is previewable or not, I think that depends on what we expect from previews. Personally I think it would already help if the view shows the filters, and media items to see what the filters look like or what the view mode looks like in the library widget. The fact that the select boxes and "Insert selected" button won't work should not block users from seeing that imho.

We should probably add more explicit tests to confirm the view is actually shown as expected:
- assert 1 or more filters
- create a media item and assert it is rendered in the view
- maybe the "Insert selected" button?

Added a patch with:

1. Only catching the exception when in preview mode
2. Added a @todo to the issue that will provide the Views API for checking preview mode in the future.
3. Created a separate test for checking views related stuff. The patch in #5 adds this to the ::testWidgetAccess test, which is about testing access. I think that's confusing and it should go in a views test.
4. Tested: filtering, checking the items in the view and clicking the "Insert Selected" button.
5. Asserted that the latter will result in an unfinshed Ajax Call with a runtime exception.

Considering the precedent of things being broken in Views Preview (e.g. bulk operations), we could accept that this results in an ajax error in the browser, since it doesn't actually affect the UI. But do me that doesn't feel right.

I think we have the following options:

1. Leave it like this
2. Leave it like it is, but add a @todo and a a follow up issue where we add a 'isPreview()' check to the actual widget form, to avoid ajax errors, and possibly add a warning: 'There is nothing to insert this item into'.
3. Add that check already in this issue.
4. Create a follow up issue for Views to provide us with an API that we can use mark certain behaviour as 'disabled in preview' or something like that.

About the interdiff:

1. +++ b/core/modules/media_library/media_library.module
   @@ -71,9 +71,10 @@
   +      // previewing.
   
   +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -26,7 +26,13 @@
   +    'views_ui',
   

   This is almost what @phenaproxima suggested.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -26,7 +26,13 @@
   +  ];
   

   Tests were failing due to missing dependencies.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -222,8 +228,8 @@
   +    ] + $route_bundle_params);
   

   Some things code sniffer picked up...I should probably revert this for the sake of clarity.

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -276,38 +282,36 @@
   +    $this->expectException(\RuntimeException::class);
   

   Instead of trying to catch the exception, we should probably use the expectException method here.

Another two concerns I have about leaving the AJAX error here:

The Path returns an access denied, instead of a 500 error. Is this right? There are missing parameters, maybe that should mean that it has to return a different response code. I realise that's a whole different issue, but worth asking here.
The log is registering the 403 response when people click the button. Clogging up the log with this might not be desirable.

The test fails, because on tear down there's a check for any pending ajax requests. Ours is stuck, so this test always fails, unless we override the tear down method. This is the only way I could figure out how to fix it. Love to hear alternative approaches.

This is becoming a bigger and bigger code smell, that makes me think we should not allow this to break in the first place, even in preview mode.

After discussing this with @phenaproxima and @seanB we come to the following conclusions:

1. Clicking the 'Insert selected' button in preview mode leads to an AJAX exception that we expect because there is nothing to insert into. It looks like it's going to be very difficult to test that: even though we can catch run time exceptions in the test, the ::tearDown would throw another runtime exception because of an unfinished AJAX call.
2. We could disable the 'Insert selected' button in preview mode, and test that, but then the Preview would be different from the actual widget which is not disirable either.
3. If views provides an API for marking certain elements of a view as 'unpreviewable', we could use that and test the result of that. See https://www.drupal.org/project/drupal/issues/3060852. Until then we simply don't test the possibility of clicking the 'Insert selected' button in preview mode.

Using forms in views previews is a problematic thing. For example the bulk operation forms in content view also produces unexpected behaviour. My personal feeling is hiding or disabling the button could also lead to confusion, since the preview is not showing exactly the same output as it would outside of preview mode.

Interacting with the preview is another story. I think it would helpful to show a message to users that interacting with preview might lead to unexpected results or something? This is probably something that needs to be discussed with the UX team, but the "interacting with previews" issue seems to be much broader than the media library widget displays. Not sure if that is best addressed in #3060852: Allow view displays to opt out of previewability or in a separate followup issue.

Fixed a stupid mistake and some coding standards.

1. Fixed
2. Fixed
3. D'oh. Fixed
4. No idea how to do this differently either
5. OK
6. Ive added the exact same test for the table widget as well.

+++ b/core/modules/media_library/media_library.module
@@ -70,7 +70,24 @@ function media_library_views_post_render(ViewExecutable $view, &$output, CachePl
+      // MediaLibraryState::fromRequest() will throw an exception if the view
+      // is being previewed, since not all required query parameters will be
+      // present. In a preview, however, this can be omitted since we're merely
+      // previewing.
+      // @todo Use the views API for checking for the preview mode when it
+      //   lands. https://www.drupal.org/project/drupal/issues/3060855
+      if (!empty($view->preview) || !empty($view->live_preview)) {
+        try {
+          $query = MediaLibraryState::fromRequest($view->getRequest())->all();
+        }
+        catch (InvalidArgumentException $e) {
+          return;
+        }
+      }
+      else {
+        $query = MediaLibraryState::fromRequest($view->getRequest())->all();
+      }

I think this code is a bit complex and having the two calls to MediaLibraryState::fromRequest() so close to each other is a bit odd.

We can do

      try {
        $query = MediaLibraryState::fromRequest($view->getRequest())->all();
      }
      catch (InvalidArgumentException $e) {
        // MediaLibraryState::fromRequest() will throw an exception if the view
        // is being previewed, since not all required query parameters will be
        // present. In a preview, however, this can be ignored since we're
        // merely previewing.
        // @todo Use the views API for checking for the preview mode when it
        //   lands. https://www.drupal.org/project/drupal/issues/3060855
        if (empty($view->preview) && empty($view->live_preview)) {
          throw $e;
        }
      }

Thanks for the tip @alexpott. This is indeed much cleaner.

Forget that last one...

Committed and pushed 6e594c260b to 8.8.x and 699a4314df to 8.7.x. Thanks!