Views preview not working without saving new display

Sent this issue to mpdonadio

OK, I can reproduce this, but haven't managed to get a full stack trace to see exact where this is barfing (I assume its the getUrl in ViewUI::renderPreview()). I suspect it will also happen when you change the display ID.

I see three options here.

1. Disable preview for unsaved view displays.
2. Autosave the route info when you set the path.
3. Have a special route for unsaved page displays that gets used for previews.

Or maybe we just need to add some try/catch in this method?

I'll play with this.

Thank you!!

OK, here is an initial assessment

ViewsForm::buildForm() and ViewsExposedForm::buildForm() both have

$form['#action'] = $view->hasUrl() ? $view->getUrl()->toString() : Url::fromRoute('<current>')->toString();

in them (and ViewsForm.php is missing a use for Url, which causes an additional fatal...) So, I think we need to update ViewExecutable::hasUrl() to check to see if $display_id has actually been saved or not.

Then ViewUI::renderPreview() has a \Drupal::l($path->toString()) in it. So, we need to adjust the logic to see if the $view has been saved or not.

If i put in hacks around those three places, the preview renders out.

First pass at an ugly fix.

@mpdonadio
I guess we should add a new field to displays on View, which simply indicates whether the display is new.

Having trouble with a version that tracks isNew through the whole process. Also, I am not sure if just tracking isNew is sufficient. If you change the machine name for a display in the UI (ie, the $display_id) and then preview, the old machine name is used in the route name for the live preview (which is what is also currently in the route table). After you save, the new route name gets used. I am not sure if this is a bug, and accident, or by design.

This is a version that uses the undocumented $view->changed and $view->live_preview variables. I am thinking that in DisplayPluginBase::initDisplay(), we could also track whether the display has changed?

(a few min goes by).

OK, I have already uploaded the patch and written this out, but it looks like $view->changed will never be set to TRUE in DisplayPluginBase::initDisplay(), and that whether the view has changed or not is actually tracked by the ViewUI class.

Straight reroll of #8 so I can post a clean interdiff.

@dawehner, do you think this approach or the one in #5 is good, or should I still pursue adding isNew()?

Its tricky. In general I prefer explicit code over implicit code ... so in this case I would prefer you rapproach in #10.
On top of that though it might be still valuable to not fatal under any circumstances.

This fixes the situation in the IS. Still need to cobble together a test. Is an AJAX thing like this testable w/ Simpletest?

Feed the TestBot or it gets cranky when its blood sugar gets low.

If you run the test-only patch via the UI or run-tests.sh in verbose mode, you see the 500 w/ the exception:

[20-Mar-2015 01:50:55 Europe/Berlin] Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "view.content.page_2" does not exist." at /Applications/MAMP/htdocs/drupal-8.0.x/core/lib/Drupal/Core/Routing/RouteProvider.php line 128

Since this is a new file, I can nitpick more

1. +++ b/core/modules/views/src/Tests/PreviewTest.php
   @@ -0,0 +1,68 @@
   + * Definition of Drupal\views\Tests\BasicTest.
   

   Contains \Drupal

2. +++ b/core/modules/views/src/Tests/PreviewTest.php
   @@ -0,0 +1,68 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   ...
   +  protected function setUp() {
   

   Both of these can use {@inheritdoc}

3. +++ b/core/modules/views/src/Tests/PreviewTest.php
   @@ -0,0 +1,68 @@
   +  public static $modules = array('node', 'views_ui');
   ...
   +  public static $testViews = array();
   

   Here and throughout, might as well use []

Once that's done I can manually test and RTBC tomorrow.

@mpdonadio++

Addressed #17, made some comments read more better, and brought the test more in line with how the other Views tests work re setup.

The array() habit is going to be a hard one to break...

1. +++ b/core/modules/views/src/Tests/PreviewTest.php
   @@ -0,0 +1,66 @@
   +class PreviewTest extends ViewTestBase {
   

   Note: We could extend UiTestBase and save a couple of lines in this file, like creating the admin user

2. +++ b/core/modules/views/src/Tests/PreviewTest.php
   @@ -0,0 +1,66 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp() {
   +    parent::setUp();
   +  }
   +
   

   This is redundant ...

#20-1 yup, missed that when I was cleaning things up. Also got rid of a unneeded use and a permission that isn't needed.

#20-2. The views_test_data module interferes with the test when I use UITestBase:

Argument 1 passed to views_test_data_views_form_substitutions() must be an instance of Drupal\views\ViewExecutable, none given

Not sure what is up with that, but I suspect it has to do with this test using a view from the node module (because that is the view from the original bug report), and not the test views.

+++ b/core/modules/views/src/Tests/PreviewTest.php
@@ -0,0 +1,57 @@
+ */
+class PreviewTest extends ViewTestBase {
+
...
+    }
+
+    $admin_user = $this->drupalCreateUser(['administer views']);
+    $this->drupalLogin($admin_user);
+

This is IMHO a clear UI test, isn't it, so I think we should use the UiTestBase and move it to core/modules/views_ui/src/Tests

OK, moved the test to views_ui and changed parent class. The hooks in views_test_data.module interfere with this test (see my comment in #21, so I tweaked the parent setUp to add a flag to enable this or not.

1. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -436,6 +436,23 @@ class ViewExecutable implements \Serializable {
   +   * If the view is currently being used for a live preview from the UI.
   +   *
   +   * @var bool
   +   */
   +  public $live_preview = FALSE;
   +
   +  /**
   +   * If the view has been changed.
   +   *
   +   * @todo It is unclear whether this is actually used, both within the class
   +   *   and outside.
   +   *
   +   * @var bool
   +   */
   +  public $changed = FALSE;
   +
   

   At least we have documented these public variables now ... though, for sure, it would be great if we could get rid of them at some point, maybe in 9.0.x :P

2. +++ b/core/modules/views_ui/src/Tests/UnsavedPreviewTest.php
   @@ -0,0 +1,56 @@
   + */
   +class UnsavedPreviewTest extends UITestBase {
   +
   

   Thank you!

+++ b/core/modules/views/src/ViewExecutable.php
@@ -436,6 +436,23 @@ class ViewExecutable implements \Serializable {
+   * @todo It is unclear whether this is actually used, both within the class
+   *   and outside.

What are we to do? A bit of searching views_ui and you can see this is used. For example:

      if (empty($view->changed)) {
        $form['changed']['#attributes']['class'][] = 'js-hide';
      }

I'll remove the @todo later today. That example did not turn up when I had searched, only uses limited to a single method.

The @todo is gone. Searching (and not relying just on Find Usages b/c the view isn't always type hinted) turned up several legitimate usages.

and not relying just on Find Usages b/c the view isn't always type hinted

I'd like that we add more and more over time, but sure, we'll never tackle all of them.

1. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1756,6 +1770,14 @@ public function hasUrl($args = NULL, $display_id = NULL) {
   +    if (!empty($this->live_preview)) {
   +      return FALSE;
   +    }
   +
   +    if (!empty($this->changed)) {
   +      return FALSE;
   +    }
   

   It would be nice to have a comment here as to why these need to be checked whilst calling the hasUrl() method.

2. +++ b/core/modules/views_ui/src/Tests/UITestBase.php
   @@ -35,10 +35,18 @@
   +  /**
   +   * Sets up a Drupal site for running functional and integration tests.
   +   *
   +   * @param boolean $enable_views_test_module
   +   *   Whether views_test_data.module should be enabled or not.
   +   */
   +  protected function setUp($enable_views_test_module = TRUE) {
        parent::setUp();
    
   -    $this->enableViewsTestModule();
   +    if ($enable_views_test_module) {
   +      $this->enableViewsTestModule();
   +    }
   
   +++ b/core/modules/views_ui/src/Tests/UnsavedPreviewTest.php
   @@ -0,0 +1,56 @@
   +class UnsavedPreviewTest extends UITestBase {
   

   Let's just extend ViewTestBase and then we don't have to make the changes to UITestBase

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -598,7 +598,7 @@ public function renderPreview($display_id, $args = array()) {
+      if ($executable->hasUrl() && $executable->display_handler->getOption('path')) {
         $path = $executable->getUrl();
       }

Can $path = $executable->getUrl() ever be hit - $executable->live_preview is set to TRUE at the beginning of this method.

OK, #31 should be addressed. Talked to @dawehner in IRC, and we want the test to still live in views_ui.

Yes, `$path = $executable->getUrl()` gets reached. Verified with debug.

I'm still not fully convinced that $view->changed is propagated everywhere it needs to be, but I left it in the code. If the view has changed, one or more displays may not have valid routes yet. We can file a followup for this.

Blerg.

@mpdonadio but how is that line being hit we set $executable->live_preview = TRUE; just above so $executable->hasUrl() should be returning FALSE - or am I mad?

@alexpott, my bad. I misread your last comment in #31. You are correct, `$executable->hasUrl()` will always return FALSE in this case because of the `$executable->live_preview = TRUE`, so `$path = $executable->getUrl();` will never be reached, so there is dead code in that function.

Let confer with @dawehner again about how we want to approach this.

@dawehner and discussed this in IRC, and decided to reformulate the approach, which will require some more debugging as to why some things aren't happening.

Maybe ViewExecutable::hasUrl() should just look up the route name on the display to see if it exists yet?

+++ b/core/modules/views_ui/src/Tests/UnsavedPreviewTest.php
@@ -0,0 +1,73 @@
+
+    $this->drupalGet('admin/structure/views/view/content');
+    $this->assertResponse(200);
+
+    $this->drupalPostForm(NULL, [], t('Add Page'));
+    $this->assertResponse(200);
+
+    $this->drupalGet('admin/structure/views/nojs/display/content/page_2/path');
+    $this->assertResponse(200);
+
+    $this->drupalPostForm(NULL, ['path' => 'foo'], t('Apply'));
+    $this->assertResponse(200);
+
+    $this->drupalPostForm(NULL, [], t('Update preview'));
+    $this->assertResponse(200);

DO you mind saving the view and then check whether the link appears?

Adjusted test. Added check to make sure the path isn't on the page when it is unsaved, and there when it is saved. Works fine for me locally.

Thank you!

The new approach seems nice and clean to me, +1. (And thanks for the clear issue report @koence.)

+++ b/core/modules/views/src/ViewExecutable.php
@@ -437,6 +438,20 @@ class ViewExecutable implements \Serializable {
+   * If the view is currently being used for a live preview from the UI.
+   *
+   * @var bool
+   */
+  public $live_preview = FALSE;
+
+  /**
+   * If the view has been changed.
+   *
+   * @var bool
+   */
+  public $changed = FALSE;
+
+  /**

So adding this documentation is out of scope now since they aren't used in the patch... can we remove this hunk?

Other than that I think this is ready.

Yup. Those are unrelated to the patch now. Gone.

Fair point!

Let's get the more simple version in.

Reposting these patches. Something went wrong, and the retest isn't showing up in the queue anymore.

Per #46, I am putting this back to RTBC. #51 has a passing patch, and the in-betweens look like the spurious fails we have been encountering the last few days.

This issue addresses a bug and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x!

+++ b/core/modules/views_ui/src/Tests/UnsavedPreviewTest.php
@@ -0,0 +1,84 @@
+ * Tests covering Preview uf unsaved Views.

Det er norsk? Uff-da!

+++ b/core/modules/views_ui/src/Tests/UnsavedPreviewTest.php
@@ -0,0 +1,84 @@
+  public function testUsavedPageDisplayPreview() {

U-saved could be the name of a thrift store.

Both typos fixed on commit.