Views performance statistics do not properly display

Note:

      $this->render_time = Timer::stop('entity.view.preview_form');

should be

      $this->executable->render_time = Timer::stop('entity.view.preview_form');

Adds a novice tag.

Please review.
Thanks.

@dawehner,i think the above assignment is wrong because the return type of Timer::stop('entity.view.preview_form'); is array type but of $render_time is int.Please give you opinion.

just fix corrupted endline in patch

Thank you @zealfire

Now it would be nice if we could have some kind of testcoverage.

I am unable to reproduce this issue. After imported the view using settings in #2 and place its block in the sidebar I do not get the notice reported in the summary, instead I am getting a different notice that says:
Notice: Undefined index: #rows in Drupal\views\Plugin\views\display\DisplayPluginBase->elementPreRender() (line 2064 of /Users/ccortes/Projects/drupalcon/sprint/drupal8/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php).

The steps I am following to reproduce this issue are:

1. Install Drupal.
2. Go to reports/dblog check that the notice message in the summary is not displayed.
3. Import the view using configuration settings from #2.
4. Go to reports/dblog check that the notice message in the summary is not displayed yet.
5. Place the "Public Exhibitors" view block in the sidebar.
6. Go to reports/dblog check that the notice message is not displayed in the report.
7. Go to the home page by clicking the "Back to site" button so the view is render.
8. Go to reports/dblog check that the notice message is displayed in the report. The notice in the summary is not displayed instead a different one is displayed twice (see attached screenshot for evidence).

Additional details: Applying the patch does not remove the notice message I am getting. Also, If the "Public Exhibitors" view block is disable the notice message is not displayed anymore.

This can probably be removed since with this fix, that variable no longer exists on the ViewUI class.

  /**
   * How long the view takes to render.
   *
   * @var int
   */
  public $render_time;

Er, actually, render_time isn't defined on the ViewExecutable class either, so the fix here should probably consistently use $this->render_time instead of $this->executable->render_time.

Patched on ViewUI.

Oops, wrong patch.

Right fix.

Still needs tests - no?

I'm working on a test (good thing, because it's uncovering several issues).

So, statistic display is currently tied very tightly to query display, to the point that it doesn't display at all unless queries are also displayed.

I've updated the issue to indicate this (the php notice is just a symptom of further broken logic).

+++ b/core/modules/views_ui/src/Tests/PreviewTest.php
@@ -90,6 +90,23 @@ function testPreviewUI() {
+    // Statistics and query.
+    $settings->set('ui.show.sql_query.enabled', TRUE)->save();
+    $this->drupalPostForm(NULL, $edit = array('view_args' => '100'), t('Update preview'));
+    $this->assertText(t('Query build time'));
+    $this->assertText(t('Query execute time'));
+    $this->assertText(t('View render time'));

Should we not test that the query is somehow displayed? Its confusing that we have twice the same asserts

Per #24, added tests to check if Query is shown or not with different settings.

Well + the actual query, not just the title?

plus the actual query now.

Great, thank you. IMHO this is enough

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -45,27 +45,13 @@ class ViewUI implements ViewEntityInterface {
-   * How long the view takes to build.
...
-   * @var int
-   */
-  public $build_time;
...
-   * How long the view takes to execute.
-   *
-   * @var int
-   */
-  public $execute_time;

Sooo, technically, these are public properties on ViewUI. But they are never used nor set in core, and here they are only being declared as null. Therefore, these hunks are just essentially removing wrong class documentation. If other code sets public properties on a ViewUI instance, well ballyhoo, good for them. It shouldn't affect their code at all since Views doesn't do anything with it ever.

So this looks okay for 8.0.x to me.

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -832,13 +818,13 @@ public function renderPreview($display_id, $args = array()) {
+      elseif ($show_stats) {
+        $table['#rows'] = $rows['statistics'];
+      }
...
-    elseif ($show_stats === 'above' || $show_stats === 'below') {
-      $table['#rows'] = $rows['statistics'];
-    }

What is really confusing about this change is that the whole outer if ($show_location === 'above' || $show_location === 'below') { is unnecessary Since the only possible values are above or below. Let's remove that here now we're adding testing.

Also it's be great if the testing could be improved to cover both the above and below cases.

Bit of a clean up as suggested in #30. Also took out the double check for $show_stats and $show_location since both contain the same information if $show_stats is set, so testing only $show_location should be enough.

Added a test for above and below. Not sure if this is the most elegant way to do it, but seems to work.

1. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -45,27 +45,13 @@ class ViewUI implements ViewEntityInterface {
   +   * @var numeric
   

   IMHO we should use float here.

2. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -827,26 +813,25 @@ public function renderPreview($display_id, $args = array()) {
   +    // The info rows are are added to $rows['query']. $rows['query'] contains
   +    // only the info rows if $show_query is not set.
   +    if ($show_info || $show_query) {
   +      $table['#rows'] = array_merge($table['#rows'], $rows['query']);
        }
   -    elseif ($show_stats === 'above' || $show_stats === 'below') {
   -      $table['#rows'] = $rows['statistics'];
   +    if ($show_stats) {
   +      $table['#rows'] = array_merge($table['#rows'], $rows['statistics']);
        }
    
   

   So I'm wondering whether we need the if in the first place. Can't we provide a default empty $rows['query'] bit?

#33.1 Timer returns a float, so yeah that makes more sense.
#33.2 Cleaned it up a bit, initialised the arrays before they get filled so they can just be merged later without worrying about what's in there.

Also cleaned up the ifelse since, like @alexpott pointed out in #30, 'above' and 'below' are the only options, seems pointless to specifically test for both, one should be enough.

Nice, this is much better now!

Unrelated fail, back to RTBC.

We're removing public properties that do nothing (and because of this I think it okay to remove), making the code easier to understand, fixing a bug and adding a test. Nice.

Committed 5fd3567 and pushed to 8.0.x and 8.1x. Thanks!