Updated: Comment 27

Problem/Motivation

The ajax controller code was pretty much untested so a load() call is still assumes to load multiple views.

Proposed resolution

Add a proper test coverage and use $entity_storage->load() as intended/

Remaining tasks

User interface changes

API changes

Original report by @RdeBoer

Glossary VIew with "Use AJAX: Yes" produces multiple errors in watchdog. Steps to reproduce are below.

8.x checkout 30 July 2013:
git clone --branch 8.x http://git.drupal.org/project/drupal.git

Enable the canned View named "Glossary". It has AJAX on by default.

Visit the View at /glossary. Output looks fine. At the top of the output we see hyperlinked letters of the alphabet. Click on one. AJAX wheel spins for a while. Nothing changes.

Errors as per attachment.
The first one that occurs (the bottom one in the attachment) is:
Warning: array_flip(): Can only flip STRING and INTEGER values! in Drupal\Core\Config\Entity\ConfigStorageController->loadMultiple() (line 125 of /Applications/drupal8/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php).

In the advanced fieldset set "Use AJAX: No" and all is good. No errors (but no AJAX).

Files: 
CommentFileSizeAuthor
#48 2053519.48.patch688 bytesalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,179 pass(es).
[ View ]
#40 vdc-2053519-40.patch11.1 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,639 pass(es).
[ View ]
#40 interdiff.txt745 bytesdawehner
#38 vdc-2053519-38.patch11.29 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to create checkout database.
[ View ]
#38 interdiff.txt717 bytesdawehner
#30 view_ajax-2053519-29.patch11.28 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,993 pass(es).
[ View ]
#30 interdiff.txt764 bytesdawehner
#27 views_ajax-2053519-27.patch11.22 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,134 pass(es).
[ View ]
#27 interdiff.txt1.01 KBdawehner
#25 views_ajax-2053519-25.patch11.22 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,022 pass(es).
[ View ]
#25 interdiff.txt1.01 KBdawehner
#23 vdc-2053519-23.patch11.23 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,400 pass(es).
[ View ]
#23 interdiff.txt1.62 KBdawehner
#19 views_ajax-2053519-19.patch11.11 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,704 pass(es).
[ View ]
#19 interdiff.txt979 bytesdawehner
#16 view_ajax-2053519-16.patch11.1 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,581 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#16 interdiff.txt1.31 KBdawehner
#14 vdc-2053519-14.patch11.31 KBmondrake
PASSED: [[SimpleTest]]: [MySQL] 58,526 pass(es).
[ View ]
#14 interdiff_10-14.txt1.54 KBmondrake
#11 Screen Shot 2013-08-26 at 7.47.05 AM.png18.89 KBlarowlan
#10 vdc-2053519-10.patch11.31 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,284 pass(es).
[ View ]
#10 interdiff.txt1.64 KBdawehner
#6 vdc-2053519-6.patch11.88 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#2 2053519-view_ajax-2.patch710 bytesmondrake
PASSED: [[SimpleTest]]: [MySQL] 57,942 pass(es).
[ View ]
Glossary View AJAX error.jpg133.62 KBRdeBoer

Comments

Issue summary:View changes

added error msg

Priority:Normal» Major
Issue tags:+VDC

This seems to be a general problem if you set 'Use AJAX: Yes' on any view.

StatusFileSize
new710 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,942 pass(es).
[ View ]

This patch seems to fix the issue. Let's see if we can add tests.

Title:AJAX errors on Glossary View with "AJAX: Yes"AJAX fails on View with "AJAX: Yes"
Status:Active» Needs review

Issue tags:+Needs tests

This needs seriously needs a functional test. If noone comes back to it, I will do it during the week.

I tried tests but I am giving up. Again, I can't find adequate drupalGetAJAX/drupalPostAJAX combinations to use to emulate AJAX in simpletest, like in #2048309: Views UI Preview - navigation is broken.

Just for reference if anyone wants to pick up, this is how far I got to in a test method in Drupal\views\Tests\Plugin\PagerTest.php:

  /**
   * Tests the normal pager, with AJAX navigation.
   */
  public function testNormalPagerAJAX() {
    // Create 11 nodes and make sure that everyone is returned.
    // We create 11 nodes, because the default pager plugin had 10 items per page.
    for ($i = 0; $i < 11; $i++) {
      $this->drupalCreateNode();
    }
    // Login.
    $admin_user = $this->drupalCreateUser(array('administer views', 'administer site configuration'));
    $this->drupalLogin($admin_user);
    // Get full pager test view for edit.
    $this->drupalGet('admin/structure/views/view/test_pager_full/edit');
    // Add a page display.
    $edit = array( );
    $this->drupalPost('admin/structure/views/view/test_pager_full/edit', $edit, t('Add Page'));
    // Set path.
    $edit = array(
      'path' => 'test_pager_full',
    );
    $this->drupalPost('admin/structure/views/nojs/display/test_pager_full/page_1/path', $edit, t('Apply'));
    // Set AJAX on.
    $edit = array(
      'use_ajax' => TRUE,
    );
    $this->drupalPost('admin/structure/views/nojs/display/test_pager_full/page_1/use_ajax', $edit, t('Apply'));
    // Save view.
    $edit = array( );
    $this->drupalPost('admin/structure/views/view/test_pager_full/edit', $edit, t('Save'));
    // Get full pager test view results.
    $result = $this->drupalGet('test_pager_full');
    // AJAX navigate to next page.
    $elements = $this->xpath('//li[contains(@class, :class)]/a', array(':class' => 'pager-next'));
    $ajax_settings = array(
      'url' => $elements[0]['href'],
      'wrapper' => 'view-test-pager-full',
      'method' => 'replaceWith',
    );
    $result = $this->drupalPostAJAX(NULL, array(), NULL, NULL, array(), array(), NULL, $ajax_settings);
  }

Priority:Major» Critical
StatusFileSize
new11.88 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

This is from my perspective a critical bug.

This adds unit tests as well as fixes some bugs beside the main part of the issue.

Issue tags:+phpunit

Add another tag.

Status:Needs review» Needs work

The last submitted patch, vdc-2053519-6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.64 KB
new11.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,284 pass(es).
[ View ]

ha.

StatusFileSize
new18.89 KB
  1. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewAjaxTest.php
    @@ -0,0 +1,78 @@
    +    // Ensure that the view insert commando is part of the result.

    %s/commando/command

  2. +++ b/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php
    @@ -0,0 +1,195 @@
    +  use Drupal\views\Ajax\ViewAjaxResponse;
    +  use Drupal\views\Controller\ViewAjaxController;

    Wrong indentation

  3. +++ b/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php
    @@ -0,0 +1,195 @@
    +// @todo Remove this once the constant got converted.
    ...
    +  // @todo Remove once drupal_static got converted to something else.

    %s/got/has been

Manually tested on simplytest.me as per steps to reproduce, fixes the OP (screenshot below).

Screen Shot 2013-08-26 at 7.47.05 AM.png

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.54 KB
new11.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,526 pass(es).
[ View ]

Just #11

Status:Needs review» Needs work
  1. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewAjaxTest.php
    @@ -0,0 +1,78 @@
    +use Symfony\Component\Routing\Generator\UrlGeneratorInterface;

    Unused statement.

  2. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewAjaxTest.php
    @@ -0,0 +1,78 @@
    +    $this->httpClient = $this->container->get('http_default_client');

    Where are we using this?

  3. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewAjaxTest.php
    @@ -0,0 +1,78 @@
    +    $data = json_decode($response);

    Why not Drupal\Component\Utility\Json::decode()?

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
new11.1 KB
FAILED: [[SimpleTest]]: [MySQL] 58,581 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

Good points.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

It is a critical bug with double test coverage. I am unable to find anything to object. @larowlan has also reviewed the patch. So I think it is safe to RTBC :).

Status:Reviewed & tested by the community» Needs work

The last submitted patch, view_ajax-2053519-16.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new979 bytes
new11.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,704 pass(es).
[ View ]

It simply can't be right at the first time.

Status:Reviewed & tested by the community» Needs review
Issue tags:+Needs tests
  1. +++ b/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php
    @@ -0,0 +1,195 @@
    +// @todo Remove this once the constant has been converted.

    We can link this to the issue I opened for that, Not sure what it as atm though :)

  2. +++ b/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php
    @@ -0,0 +1,195 @@
    +   * Setups a bunch of valid mocks like the view entity and executable.

    Sets up

  3. +++ b/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php
    @@ -0,0 +1,195 @@
    +
    +namespace {

    We should put the obligatory @todo here to remove when we can.

Issue tags:-Needs tests

Oh, sorry, I didn't reload the issue! me--

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.62 KB
new11.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,400 pass(es).
[ View ]

There we go.

+++ b/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php
@@ -179,13 +180,14 @@ protected function setupValidMocks() {
-  // @todo Remove once drupal_static got converted to something else.
+  // @todo Remove once drupal_render got converted to a autoloadable code.

Please revert this.

StatusFileSize
new1.01 KB
new11.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,022 pass(es).
[ View ]

That is just part of the interdiff...

Status:Needs review» Reviewed & tested by the community

// @todo Remove once drupal_render IS converted to autoloadable code.

Sorry, small. RTBC apart from that.

StatusFileSize
new1.01 KB
new11.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,134 pass(es).
[ View ]

There we go.

Perfect, didn;t actually mean to set RTBC before but +1 on my own premature RTBC.

Issue summary:View changes

mention Git command

+++ b/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.php
@@ -94,11 +95,11 @@ public function ajaxView(Request $request) {
       $view = $this->executableFactory->get($entity);
+      $response->setView($view);
       if ($view && $view->access($display_id)) {

Lets move the $response->setView($view); inside the if

StatusFileSize
new764 bytes
new11.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,993 pass(es).
[ View ]

Moved to the inner if.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php
@@ -0,0 +1,197 @@
+  // @todo Remove once drupal_static is converted to autoloadable code.

drupal_static() is never going to be straight converted to autoloadable code. Other code being converted to services will eventually mean it's no longer needed.

drupal_static() is never going to be straight converted to autoloadable code. Other code being converted to services will eventually mean it's no longer needed.

Well, drupal_static is used to change the drupal_get_destination(). Do you have a better suggestion what the comment should be?

Status:Needs work» Needs review

#30: view_ajax-2053519-29.patch queued for re-testing.

Status:Needs review» Needs work

Setting back to needs work for #31 and #32

Status:Needs work» Reviewed & tested by the community

I still think that we can't do anything against it for now.

Assigned:Unassigned» catch

Bouncing back to catch.

Status:Reviewed & tested by the community» Needs review

I didn't mean getting rid of drupal_static() from here, just fixing the comment. Something like 'Remove when drupal_static() is no longer needed to change drupal_get_destination()"?

StatusFileSize
new717 bytes
new11.29 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to create checkout database.
[ View ]

Oh I see. Thank you!

Status:Needs review» Needs work

+++ b/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php
@@ -0,0 +1,197 @@
+// @todo Remove this once the constant has been converted, see
+// https://drupal.org/node/2067017.
+if (!defined('DRUPAL_CORE_COMPATIBILITY')) {
+ define('DRUPAL_CORE_COMPATIBILITY', '8.x');
+}
+

#2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants was committed, shall this be changed now?

StatusFileSize
new745 bytes
new11.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,639 pass(es).
[ View ]

There were *2* identical comments :/ , only one was changed...

Well ... the one about drupal_render() is actually true as it is.

Good catch!

Status:Needs work» Needs review

... :)

You've been quicker than my re-read.... so my edit came too late...

Cheers

Status:Needs review» Needs work
Issue tags:-phpunit, -VDC

The last submitted patch, vdc-2053519-40.patch, failed testing.

Status:Needs work» Needs review

#40: vdc-2053519-40.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, vdc-2053519-40.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+phpunit, +VDC

#40: vdc-2053519-40.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

All feedback addressed, back to RTBC

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Title:AJAX fails on View with "AJAX: Yes"Followup: Broken PHPUnit test with notices enabled for AJAX fails on View with "AJAX: Yes"
Status:Fixed» Needs review
StatusFileSize
new688 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,179 pass(es).
[ View ]

I'm getting a test failure with this new code.

phpunit --filter 'ViewAjaxControllerTest'
PHPUnit 3.7.21 by Sebastian Bergmann.
Configuration read from /Volumes/devdisk/dev/drupal/core/phpunit.xml.dist
...E
Time: 1 second, Memory: 31.25Mb
There was 1 error:
1) Drupal\views\Tests\Controller\ViewAjaxControllerTest::testAjaxView
Only variables should be assigned by reference
/Volumes/devdisk/dev/drupal/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.php:122
/Volumes/devdisk/dev/drupal/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php:136
/usr/local/php5-5.3.25-20130615-024255/lib/php/PHPUnit/TextUI/Command.php:176
/usr/local/php5-5.3.25-20130615-024255/lib/php/PHPUnit/TextUI/Command.php:129
FAILURES!
Tests: 4, Assertions: 7, Errors: 1.

It's happening because the replacement drupal_static in ViewAjaxControllerTest does not return a reference as it should.

Status:Needs review» Reviewed & tested by the community

I remember writing that code and thought: This can't make a difference :(

Issue tags:+quickfix

.

Status:Reviewed & tested by the community» Fixed

Shoot, I am so sorry. :(

Looks like someone already committed this.

Title:Followup: Broken PHPUnit test with notices enabled for AJAX fails on View with "AJAX: Yes"Broken PHPUnit test with notices enabled for AJAX fails on View with "AJAX: Yes"

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

VDC