Follow-up to #2472323: Move modal / dialog to query parameters

Problem/Motivation

As noted in the issue this follows up, "we use accept headers both for routing, but also for a way to convert the result of the current controller into a different format like a modal, dialog or ajax". The issue was closed without all of the problems fixed, notably the third item:

  • Convert the various tests to use query parameters ...

AJAX Views tests in core/modules/views/src/Tests/ViewAjaxTest.php need to be updated so that any AJAX responses with an accept header of "application/vnd.drupal-ajax" are replaced with the query parameter _wrapper_format. As of this writing, there is only one response request that needs the fix.

Proposed resolution

In core/modules/views/src/Tests/ViewAjaxTest.php, look for "application/vnd.drupal-ajax." (grep -Ern "vnd\.drupal" core). Then do the following:

1. Replace 'application/vnd-drupal-ajax' with an empty string:
(e.g.)
$response = $this->drupalPost($quickedit_uri, 'application/vnd.drupal-ajax', $post);
becomes
$response = $this->drupalPost($quickedit_uri, '', $post);

2. Add query string array to the end of drupalPost. e.g.
$response = $this->drupalPost($quickedit_uri, '', $post);
becomes
$response = $this->drupalPost($quickedit_uri, '', $post, array('query' => array(MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax')));

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Novice Instructions No
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Novice Instructions No

User interface changes

None

API changes

None

CommentFileSizeAuthor
#19 reroll-2488710-19.patch934 bytesvg3095
#19 reroll-2488710-19.patch934 bytesvg3095
#12 views-update_test_in_views_module-2488710-12.patch960 bytesgreendemiurge
#12 interdiff-10-12.txt821 bytesgreendemiurge
#10 views-update_test_in_views_module-2488710-10.patch925 bytesericski
#8 views-update_test_in_views_module-2488710-8.patch925 bytesericski
#3 views-update_test_in_views_module-2488710-3.patch728 bytesgreendemiurge

Comments

wheatpenny’s picture

wheatpenny commented

Current line number: core/modules/views/src/Tests/ViewAjaxTest.php:54:

greendemiurge’s picture

greendemiurge commented

I will begin work on this as part of the Drupalcon 2015 Core mentored sprint.

greendemiurge’s picture

greendemiurge commented
Status: Needs work » Needs review
StatusFileSize
new views-update_test_in_views_module-2488710-3.patch728 bytes

This patch implements the solution detailed above.

jasonmce’s picture

jasonmce commented

I'm going to take a swing at reviewing this as part of the Drupalcon 2015 Core mentored sprint.

ericski’s picture

ericski commented
Status: Needs review » Reviewed & tested by the community

RTBC

Patch applies cleanly on current HEAD, fixes issue as described in proposed solution.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: views-update_test_in_views_module-2488710-3.patch, failed testing.

ericski’s picture

ericski commented

Waiting on information coming from issue #2488696: Update tests in editor module before revisiting this issue

ericski’s picture

ericski commented
Status: Needs work » Needs review
StatusFileSize
new views-update_test_in_views_module-2488710-8.patch925 bytes

Original patch was close, just needed to add a use statement for MainContentViewSubscriber class

Status: Needs review » Needs work

The last submitted patch, 8: views-update_test_in_views_module-2488710-8.patch, failed testing.

ericski’s picture

ericski commented
Status: Needs work » Needs review
StatusFileSize
new views-update_test_in_views_module-2488710-10.patch925 bytes

Oops! did a reverse patch, sorry about that, here's the right one!

greendemiurge’s picture

greendemiurge commented

I am going to update this to make it conform to coding standards.

greendemiurge’s picture

greendemiurge commented
StatusFileSize
new interdiff-10-12.txt821 bytes
new views-update_test_in_views_module-2488710-12.patch960 bytes

This re-rolls #2488710-10: Update AJAX requests in views module tests to put the array on multiple lines in according with the coding standards: https://www.drupal.org/coding-standards#array.

jasonmce’s picture

jasonmce commented

Patch at #2488710-12: Update AJAX requests in views module tests performed as expected. Waiting for Testbot to agree, and then I will mark it RTBC.

Status: Needs review » Needs work

The last submitted patch, 12: views-update_test_in_views_module-2488710-12.patch, failed testing.

jasonmce’s picture

jasonmce commented
Status: Needs work » Reviewed & tested by the community

Whitebox tested : stepped through execution with @greendemiurge. Verified that new code passes more appropriate values to drupalPost(). Response matched expected results and passed final validation.

ericski queued 12: views-update_test_in_views_module-2488710-12.patch for re-testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
Related issues: +#2488696: Update tests in editor module, +#2488448: Update tests in QuickEdit module

Thanks for the patch and also especially for documenting your testing steps.

The issue summary here doesn't entirely help me understand why this change is needed. I also checked the parent issue in #2472323: Move modal / dialog to query parameters and didn't see anything there about this issue being filed. So tagging for an issue summary update.

I showed this to @dawehner and he is also not sure it is the correct fix.

Also, in general, it would be better to make all the fixes for one conceptual scope in one patch, rather than splitting up patches per module (as is being done with the related issues #2488696: Update tests in editor module and #2488448: Update tests in QuickEdit module). Would it make sense to merge these into one patch, since they're all essentially the same change and would have the same summary?

Finally, the title of this issue is terrifying. ;) Let's update it for what specifically we need to update.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Update tests in views module » Update AJAX requests in views module tests

Something like that maybe.

vg3095’s picture

vg3095 commented
Status: Needs work » Needs review
StatusFileSize
new reroll-2488710-19.patch934 bytes
new reroll-2488710-19.patch934 bytes

Patch did not apply 

C:\xampp\htdocs\drupal>git apply --check views-update_test_in_views_module-2488710-12.patch
error: patch failed: core/modules/views/src/Tests/ViewAjaxTest.php:51
error: core/modules/views/src/Tests/ViewAjaxTest.php: patch does not apply

So I updated the patch.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

petedussin’s picture

petedussin commented
Issue summary: View changes

Updating the Issues summary as per xim's request. The parent issue was already closed, and the work for parallel issue Update tests in editor module (2488696) is in progress as well.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

zeip’s picture

zeip commented
Status: Needs review » Closed (duplicate)
Related issues: +#2502865: Remove all remaining usages of the drupal_ajax accept header

Seems to me that these changes have already been done in commit 22c915f3 for issue #2502865: Remove all remaining usages of the drupal_ajax accept header. Closing as duplicate.