Uncaught PHP Exception LogicException when editing Views

This is certainly a critical, given that the views UI is pretty much entirely broken!

This is the first problem I found.

Manually tested and the patch fixes the issue. Settings to NW for missing tests.

Working on test coverage

A bug/missing test coverage, uncovered thanks to #2450993: Rendered Cache Metadata created during the main controller request gets lost.

Test coverage

Without useless cruft

Let's do it right this time.

s/see/seem

Wow... that's an impressive bug. Edit: And yeah, this is critical.

Also, +1 for the render array use replacing the SafeMarkup::set() call. It's actually reassuring that that is the fix and means our larger critical objectives are actually on the right track. :)

+++ b/core/modules/views_ui/src/Tests/FieldUITest.php
@@ -57,6 +57,11 @@ public function testFieldUI() {
+    $this->assertResponse(200);

Anything worth asserting other than the 200? Content type header maybe?

Otherwise looks great.

There was a crosspost that made the result reporting goofy for the test-only patch, but it does fail locally. I also managed to dig the result out of the testbot logs:
https://qa.drupal.org/pifr/test/1088358

Edit: I crossposted with catch; +1 for adding an additional assertion.

Nit: unwanted space in

[ '#markup']

Here is an idea, "hack" drupalGet() with nojs.

+++ b/core/modules/views_ui/src/Tests/UITestBase.php
@@ -74,4 +74,21 @@ public function randomView(array $view = array()) {
+      $this->drupalGet($url, $options, $headers);

How much overhead does this add to every views UI test? Not saying I don't love the idea, but should it be opt-in rather than opt-out?

Also, should we document the extra 'no_ajax_check' option on this method instead of using {@inheritdoc}

Also what happens if there is no 'ajax' in the $path, - we'd end up with a duplicate request to the same path right? because str_replace($url, ...) would be the same as the previous $url?

Also I think I've used my quota of alsos.

1. +++ b/core/modules/views_ui/src/Tests/UITestBase.php
   @@ -74,4 +74,21 @@ public function randomView(array $view = array()) {
   +    // Ensure that each nojs page is accessible via ajax as well.
   +    if (empty($options['no_ajax_check']) && strpos($url, 'nojs') !== FALSE) {
   

   Well, here it is checking whether nojs is in there.

2. +++ b/core/modules/views_ui/src/Tests/UITestBase.php
   @@ -74,4 +74,21 @@ public function randomView(array $view = array()) {
   +      $this->drupalGet($url, $options, $headers);
   +      $this->assertResponse(200);
   

   I think the additional request is worth the additional test coverage

Whoops, yeah I misunderstood, we're going the other way (nojs » ajax), ignore me - so I think we should just document the no_ajax_check option on the overridden method.

Given that the patch is green, I think its fine for now to just skip the option.

Looking at logs the test suite execution time does not seem to be heavily affected by the additional test coverage.

Aside form that, I agree with @mpdonadio that the fact that the error happens only in the AJAX context could use some additional investigation. However I think the suspect that something more might be wrong is not enough to hold off a critical issue, so I think a non-critical follow-up would be better for that.

My tiny test coverage patch was completely superseded by the new generic test coverage, so I think it's fine for me to RTBC this.

I opened #2528124: Additional simpletest and/or manual testing of Views UI for follow-up testing.

But I still think we should check content type header or something to ensure this is a valid response. I'm sure Drupal 8 was fataling with a 200 response at one point.

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -875,10 +875,12 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+      $output = [];
+      $output[]['#markup'] = '<p>' . $this->t('You must add some additional fields to this display before using this field. These fields may be marked as <em>Exclude from display</em> if you prefer. Note that due to rendering order, you cannot use fields that come after this field; if you need a field not listed here, rearrange your fields.') . '</p>';
...
-        $output = '<p>' . $this->t("The following replacement tokens are available for this field. Note that due to rendering order, you cannot use fields that come after this field; if you need a field not listed here, rearrange your fields.") . '</p>';
+        $output[]['#markup'] = '<p>' . $this->t("The following replacement tokens are available for this field. Note that due to rendering order, you cannot use fields that come after this field; if you need a field not listed here, rearrange your fields.") . '</p>';

That is a total nit, but:

$output[]['#markup'] =

looks totally strange to my eyes ...

It creates a new element, then makes it empty, then adds #markup to it, then adds data to that.

What about:

$output[] = ['#markup' => ... ]

instead?

---

Overall except for catch' remark, RTBC + 1

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -890,7 +892,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-            $output .= $this->getRenderer()->render($item_list);

Rather than fixing this one case, of which there will likely be more in other Views plugins/handlers, I think it'd be better to update this code from:

    $form = \Drupal::formBuilder()->buildForm($form_class, $form_state);
    $output = $renderer->renderRoot($form);

to:

    // The plugins and handlers that buildForm() calls may still be doing early
    // rendering. Rather than fixing them all, execute them in a render context
    // so that EarlyRenderingControllerWrapperSubscriber does not complain.
    $render_context = new RenderContext();
    $form = $renderer->executeInRenderContext($render_context, function() {
      return \Drupal::formBuilder()->buildForm($form_class, $form_state);
    });
    BubbleableMetadata::createFromRenderArray($form)
      ->merge($render_context->pop())
      ->applyTo($form);
    $output = $renderer->renderRoot($form);

Yeah, there we go.

1. +++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
   @@ -205,8 +206,13 @@ protected function ajaxFormWrapper($form_class, FormStateInterface &$form_state)
   +    $render_context = new RenderContext();
   +    $callable = function () use ($form_class, &$form_state) {
   +      return \Drupal::formBuilder()->buildForm($form_class, $form_state);
   +    };
   +    $form = $renderer->executeInRenderContext($render_context, $callable);
   

   Inline comment would be super here.

2. +++ b/core/modules/views_ui/src/Tests/FieldUITest.php
   @@ -57,6 +57,11 @@ public function testFieldUI() {
   +    // Ensure the AJAX UI is responding.
   +    $ajax_url = 'admin/structure/views/ajax/handler/test_view/default/field/age';
   +    $this->drupalGet($ajax_url);
   +    $this->assertResponse(200);
   

   NW for the additional test assertion (see @catch's comment above).

Sure, let's do that. I removed the test coverage from FieldUITest because the generic drupalGet() one covers that already, but much better.

+++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
@@ -205,8 +206,13 @@ protected function ajaxFormWrapper($form_class, FormStateInterface &$form_state)
+    $form = $renderer->executeInRenderContext($render_context, $callable);
     $output = $renderer->renderRoot($form);

The render_context needs to be applied still to $form.

=> CNW

... this was a old comment

1. index f17df18..3d1775d 100644
   --- a/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   

   The fact that we now wrap the form building in a render context means that all changes here are now technically no longer necessary, but they're still an improvement of course.

   I'll leave it to a committer to decide whether to do the right, yet strictly speaking now unnecessary thing.

2. +++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
   @@ -205,8 +207,21 @@ protected function ajaxFormWrapper($form_class, FormStateInterface &$form_state)
   +    // Render the form in a render context in order to ensure that cacheable
   +    // metadata is bubbled up.
   

   This does NOT render the form, it builds the form. This should probably mention that this specifically wants to allow Views handlers/plugins that provide forms to still do early rendering. If they didn't do early rendering, this would not be necessary.

3. +++ b/core/modules/views_ui/src/Tests/UITestBase.php
   @@ -74,4 +74,23 @@ public function randomView(array $view = array()) {
   +    // Ensure that each nojs page is accessible via ajax as well.
   

   Oh, nice, this is very comprehensive for sure :)

I'll leave it to a committer to decide whether to do the right, yet strictly speaking now unnecessary thing.

I think fixing the UI with that is not a bad idea.

Reviews above are addressed back to RTBC.

Patch to commit is #37 (https://www.drupal.org/files/issues/2527606-37.patch).

RTBC + 1

Unrelated:

+++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
@@ -205,8 +207,21 @@ protected function ajaxFormWrapper($form_class, FormStateInterface &$form_state)
+      BubbleableMetadata::createFromRenderArray($form)
+        ->merge($render_context->pop())
+        ->applyTo($form);

I will open a follow-up to add a $render_context->applyTo() method directly.

Edit: Opened #2528322: Add RenderContext::applyTo method

Nice one less SafeMarkup::set() too! Committed c78d806 and pushed to 8.0.x. Thanks!

RTBC +1

Too late :) Congrats!

I applied just the test changes, ran a couple of Views UI tests, nothing failed.

Here's a test-only patch. I'm wondering if we got the exception because we were in an AJAX response, and then we updated the tests to use non-AJAX, so now we're not testing it at all again.

So if this issue goes to CNW, it's RTBC. And if it stays at RTBC, it's CNW ;)

I don't think I applied the patch before running the tests locally, which obviously would not fail 'cos that's just HEAD. So ignore #44 but it's always nice to have test-only patches on issues for confidence.

I can confirm Drupal\views_ui\Tests\FieldUITest fails with #44 applied.

The revert and re-commit added @catch to the list of people credited.

Interesting how the re-commit showed up before the revert. That really confused me :)