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).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RdeBoer’s picture

Issue summary: View changes

added error msg

mondrake’s picture

Priority: Normal » Major
Issue tags: +VDC

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

mondrake’s picture

FileSize
710 bytes

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

mondrake’s picture

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

Issue tags: +Needs tests

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

mondrake’s picture

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);
  }
dawehner’s picture

Priority: Major » Critical
FileSize
11.88 KB

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.

dawehner’s picture

Issue tags: +PHPUnit

Add another tag.

dawehner’s picture

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
11.31 KB

ha.

larowlan’s picture

  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

mondrake’s picture

mondrake’s picture

Status: Needs review » Needs work
mondrake’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
11.31 KB

Just #11

jibran’s picture

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()?

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
11.1 KB

Good points.

jibran’s picture

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.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
979 bytes
11.11 KB

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

damiankloip’s picture

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.

damiankloip’s picture

Issue tags: -Needs tests

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

jibran’s picture

Status: Needs review » Needs work
dawehner’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
11.23 KB

There we go.

jibran’s picture

+++ 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.

dawehner’s picture

FileSize
1.01 KB
11.22 KB

That is just part of the interdiff...

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

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

Sorry, small. RTBC apart from that.

dawehner’s picture

FileSize
1.01 KB
11.22 KB

There we go.

damiankloip’s picture

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

damiankloip’s picture

Issue summary: View changes

mention Git command

alexpott’s picture

+++ 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

dawehner’s picture

FileSize
764 bytes
11.28 KB

Moved to the inner if.

catch’s picture

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.

dawehner’s picture

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?

mondrake’s picture

Status: Needs work » Needs review

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

mondrake’s picture

Status: Needs review » Needs work

Setting back to needs work for #31 and #32

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

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

webchick’s picture

Assigned: Unassigned » catch

Bouncing back to catch.

catch’s picture

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()"?

dawehner’s picture

FileSize
717 bytes
11.29 KB

Oh I see. Thank you!

mondrake’s picture

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?

dawehner’s picture

FileSize
745 bytes
11.1 KB

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

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

Good catch!

mondrake’s picture

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.

dawehner’s picture

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.

dawehner’s picture

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

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

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

All feedback addressed, back to RTBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

alexpott’s picture

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
FileSize
688 bytes

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

dawehner’s picture

Issue tags: +quickfix

.

dawehner’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Shoot, I am so sorry. :(

Looks like someone already committed this.

webchick’s picture

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.

Anonymous’s picture

Issue summary: View changes

VDC