Updated: Comment #0

Problem/Motivation

In #914382: Contextual links incompatible with render cache and Edit.module's testing coverage, a lot of tests were written/changed to use WebTestBase::curlExec(). With a lot of parameters that are the same everywhere.

The problem is that we can't use either drupalPost() or drupalPostAjax(): both have deeply embedded assumptions that they're being applied to forms. That's an assumption that does not hold true for some things that are new in Drupal 8: when rendering contextual links, nor when retrieving Edit module's metadata, nor when Edit module makes requests to in-place edit fields. Hence we were forced to use WebTestBase::curlExec(). But now there are so many instances of calling curlExec() with nigh identical parameters that it makes sense to add a new method to WebTestBase for it.

Proposed resolution

Add WebTestBase::drupalPostCustom() (better name welcomed) and WebTestBase::getAjaxPageStatePostData().

The latter should be obvious, the former contains the keyword "custom" to indicate that you can use it for any sort of custom POST HTTP request: the caller has full control over the $post data, and can set the $accept header easily.

Remaining tasks

None.

User interface changes

None.

API changes

No changes, only two additions:

  1. WebTestBase::drupalPostCustom()
  2. WebTestBase::getAjaxPageStatePostData()

#914382: Contextual links incompatible with render cache

Files: 
CommentFileSizeAuthor
#39 drupal_post_cleanup-2074037-39.patch791.79 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,861 pass(es).
[ View ]
#37 drupal_post_cleanup-2074037-37.patch810.24 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,751 pass(es).
[ View ]
#37 interdiff.txt850 bytesWim Leers
#35 drupal_post_cleanup-2074037-35.patch810.1 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,724 pass(es), 4 fail(s), and 7 exception(s).
[ View ]
#35 interdiff.txt4.31 KBWim Leers
#31 drupal_post_custom-2074037-31.patch807.32 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_post_custom-2074037-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 interdiff.txt792.6 KBWim Leers
#22 drupal_post_custom-2074037-22.patch28.99 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,461 pass(es).
[ View ]
#22 interdiff.txt2.37 KBWim Leers
#20 drupal_post_custom-2074037-20.patch27.26 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php.
[ View ]
#20 interdiff.txt11.29 KBWim Leers
#10 drupal_post_custom-2074037-10.patch27.14 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,477 pass(es).
[ View ]
#10 interdiff.txt1.26 KBWim Leers
#6 drupal_post_custom-2074037-5.patch27.14 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,188 pass(es).
[ View ]
#6 interdiff.txt3.58 KBWim Leers
#3 drupal_post_custom-2074037-3.patch23.62 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_post_custom-2074037-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Oh, nice, #2053519: Broken PHPUnit test with notices enabled for AJAX fails on View with "AJAX: Yes" is indeed another perfect example :)

(Patch is on the way!)

Status:Active» Needs review
Issue tags:+sprint
StatusFileSize
new23.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_post_custom-2074037-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And voila. Nice diff stats: 6 files changed, 115 insertions(+), 265 deletions(-).

I grepped the code base for all existing WebTestBase::curlExec() calls and then checked if I could convert them to drupalPostCustom().
Nine instances of curlExec() were replaced with drupalPostCustom().

Status:Needs review» Needs work

The last submitted patch, drupal_post_custom-2074037-3.patch, failed testing.

ajax.js does this:

...
Drupal.ajax = function (base, element, element_settings) {
  var defaults = {
    event: 'mousedown',
    keypress: true,
    selector: '#' + base,
    effect: 'none',
    speed: 'none',
    method: 'replaceWith',
    progress: {
      type: 'throbber',
      message: Drupal.t('Please wait...')
    },
    submit: {
      'js': true
    }
  };
...

The 'js': true setting means that anytime data is sent to the server via ajax, the form state will also have a $form_state['input']['js'] key set to literal 'true'.
I used that in #2048309: Views UI Preview - navigation is broken to check if the view preview is to be refreshed, but could not build a test for that as this was not embedded in simpletest.

I wonder if we can reflect that default setting in

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -1659,6 +1648,84 @@ protected function drupalPostAJAX($path, $edit, $triggering_element, $ajax_path
+  public function getAjaxPageStatePostData() {
+    $post = array();
+    $drupal_settings = $this->drupalSettings;
+    if (isset($drupal_settings['ajaxPageState'])) {
+      $post['ajax_page_state[theme]'] = $drupal_settings['ajaxPageState']['theme'];
+      $post['ajax_page_state[theme_token]'] = $drupal_settings['ajaxPageState']['theme_token'];
+      foreach ($drupal_settings['ajaxPageState']['css'] as $key => $value) {
+        $post["ajax_page_state[css][$key]"] = 1;
+      }
+      foreach ($drupal_settings['ajaxPageState']['js'] as $key => $value) {
+        $post["ajax_page_state[js][$key]"] = 1;
+      }
+    }
+    return $post;

by making the $post default to

$post = array('js' => 'true');

and so have an aligned js and simpletest behavior?

Status:Needs work» Needs review
StatusFileSize
new3.58 KB
new27.14 KB
PASSED: [[SimpleTest]]: [MySQL] 58,188 pass(es).
[ View ]

I'd forgotten to pull in the latest. Hence the patch didn't apply.

This reroll updates a newly added instance of curlExec().

#5: Hrm… I've never, ever actually seen js = true being submitted in AJAX-ified (i.e. ajax.js-ified) XHR requests. So I think that's actually being overridden. Where is that being checked on the server side?
Of course I want aligned behavior, but then I first want to understand why it's there, because my first impression is that that particular piece of code is dead.

Status:Needs work» Needs review

#7: See http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

It seemed weird to me too, but that was the only way I could find to check if, at that stage, we are processing an ajax request or a normal GET.

Status:Needs review» Needs work

Nice!!

  1. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -179,7 +185,8 @@ function testUserWithPermission() {
           $this->assertIdentical(1, count($ajax_commands), 'The entity submission HTTP request results in one AJAX command.');
    @@ -193,184 +200,4 @@ function testUserWithPermission() {
    -    $post = implode('&', $post);
    +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -1659,6 +1648,84 @@ protected function drupalPostAJAX($path, $edit, $triggering_element, $ajax_path
    +  public function drupalPostCustom($path, $accept, array $post, $options = array()) {
    ...
    +  public function getAjaxPageStatePostData() {

    Technically they don't need to be public (you just subclass anyway), but I don't care.

  2. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/DisplayTest.php
    @@ -294,16 +294,8 @@ public function testPageContextualLinks() {
    -    $post = urlencode('ids[0]') . '=' . urlencode($id);
    -    $response = $this->curlExec(array(
    -      CURLOPT_URL => url('contextual/render', array('absolute' => TRUE, 'query' => array('destination' => 'test-display'))),
    -      CURLOPT_POST => TRUE,
    -      CURLOPT_POSTFIELDS => $post,
    -      CURLOPT_HTTPHEADER => array(
    -        'Accept: application/json',
    -        'Content-Type: application/x-www-form-urlencoded',
    -      ),
    -    ));
    +    $post = array('ids[0]' => $id);
    +    $response = $this->drupalPostCustom('contextual/render', 'application/json', $post, array('query' => array('destination' => 'test-display')));

    Way better!

StatusFileSize
new1.26 KB
new27.14 KB
PASSED: [[SimpleTest]]: [MySQL] 58,477 pass(es).
[ View ]

#8: shouldn't that look at the Accept header insted?

#9:

  1. Hah, good point :) Fixed!
  2. :)

Re. #8, I git blamed that file and found that

submit: {
      'js': true
    }

was added in #850612: setting class 'use-ajax-submit' for form submission causes js error(s), see comments #21 and #22.

I believe we could go for checking the accept header too, even if it seems to me it's not bad to have a flag that indicates if we are in an AJAX context w/o having to go so low-level.

In any case, if

submit: {
      'js': true
    }
is dead, shouldn't it be removed first?

#11: Rather than postponing this issue on getting rid of that, I'd much rather just add it here for now, then get rid of it later. I'm indeed seeing it show up on e.g. when configuring a Views field.
I don't think setting js = true belongs in getAjaxPageStatePostData(), because then the function name is no longer accurate. Futhermore, it's entirely possible to use Drupal.ajax and js = true not getting sent to the server (e.g. Edit.module's "attachments" call, which uses Drupal.ajax too). IMO this should be fixed in drupalPostAJAX(), which is always used for the scenarios where this is appropriate.
Alternatively, you could also solve this by including js = true in your $post data when calling drupalPostCustom().


I think this is ready to go? :)

#12

solve this by including js = true in your $post data when calling drupalPostCustom()

That's fine.

IMO this should be fixed in drupalPostAJAX()...

Well, the problem I was having was when I wanted to navigate in views' preview clicking pager page links, not action buttons. In this case 'clicking' a page link is not a form post, whereas drupalPostAJAX is really about forms. Clicking the link when AJAX enabled is in fact a HTTP POST and in simpletest is interpreted as a form post... vicious circle. So, yes, probably drupalPostAJAX should be 'aligned', but it would not help my case. Including js = true in post data when calling drupalPostCustom() will.

I think this is ready to go? :)

Fine for me, maybe more reviewers?

+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
@@ -142,7 +145,8 @@ function testUserWithPermission() {
-    $response = $this->retrieveFieldForm('node/1/body/und/full');
+    $post = array('nocssjs' => 'true') + $this->getAjaxPageStatePostData();
+    $response = $this->drupalPostCustom('edit/form/' . 'node/1/body/und/full', 'application/vnd.drupal-ajax', $post);
@@ -179,7 +185,8 @@ function testUserWithPermission() {
-      $response = $this->saveEntity('node/1');
+      $post = array('nocssjs' => 'true');
+      $response = $this->drupalPostCustom('edit/entity/' . 'node/1', 'application/json', $post);
@@ -193,185 +200,4 @@ function testUserWithPermission() {
-  protected function retrieveFieldForm($field_id) {
-    // Build & serialize POST value.
-    $post = urlencode('nocssjs') . '=' . urlencode('true');
...
-    // Build & serialize POST value.
-    $post = urlencode('nocssjs') . '=' . urlencode('true');

I was wondering what this nocssjs is about, but this just got moved outside of this additional helper methods.

Status:Needs review» Reviewed & tested by the community

I wanted to RTBC that.

7 files changed, 117 insertions, 311 deletions.

Well that's a lovely diffstat! :)

The drupalPostCustom() name is indeed pretty weird. If we weren't post-API freeze I'd recommend renaming drupalPost to drupalPostForm and using this function as drupalPost. Either that, or incorporate this code into drupalPost directly so that it can do either, but that would require changing the method signature, which is also an API break.

Only thing I can possibly think of is drupalPostUrl()? But not sure that fits given the variety of things you are using it for.

Leaving at RTBC for the moment for feedback on that question.

It's a wrapper around curlExec, so how about drupalPostExec()?

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

git ac https://drupal.org/files/drupal_post_custom-2074037-10.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 27793  100 27793    0     0  19256      0  0:00:01  0:00:01 --:--:-- 23633
error: patch failed: core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php:86
error: core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php: patch does not apply

drupalPostExec sadly does not tell you anything about the difference between the normal drupalPost. I really like the idea of webchick!

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new11.29 KB
new27.26 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php.
[ View ]

s/drupalPostCustom(/drupalPostUrl(

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal_post_custom-2074037-20.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.37 KB
new28.99 KB
PASSED: [[SimpleTest]]: [MySQL] 58,461 pass(es).
[ View ]

#2047659: Add test coverage for edit module access checkers landed, which led to #20 failing because it introduced more calls to the custom curlExec() wrapper methods.

Updated those new calls as well (using identical substitutions), ran tests locally, all green. Should come back green.

Title:Add drupalPostCustom() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requestsAdd drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests

Updating title :)

It's not clear from the method declaration what drupalPostUrl() does. It sounds like it is a more specialized function compared to drupalPost() but it is really the more basic function. Would it not make more sense to rename the current drupalPost() to drupalPostForm(), and to make drupalPostUrl() drupalPost()? Probably too big of a change ...

#25: Yes, exactly, the current drupalPost() function name is too generic. Everything you say makes sense, but I think doing that renaming might disrupt too many patches.
Although if you feel that's what necessary, we can do that anyway.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Approved API change

Ok, talked to Alex Pott and he agreed with Dries and I about the fact that the right way to fix this is to make drupalPost() do what this does, and make the current drupalPost that's only good for forms have "form" in the name somewhere.

Sorry for the hassle, but we don't want to break APIs twice.

Let me then suggest to touch also the current 'drupalPostAJAX' then, and have 'form' in its name as well...

Yes, that should then become drupalPostAjaxForm(). I'd like sign-off on that too from the core committers before working on this.

Make it so!

Status:Needs work» Needs review
StatusFileSize
new792.6 KB
new807.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_post_custom-2074037-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Done.

Definitely the largest interdiff I've ever posted!

Status:Needs review» Needs work
Issue tags:-clean-up, -sprint, -VDC, -Spark, -Approved API change

The last submitted patch, drupal_post_custom-2074037-31.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+clean-up, +sprint, +VDC, +Spark, +Approved API change

#31: drupal_post_custom-2074037-31.patch queued for re-testing.

Status:Needs review» Needs work

#2053519: Broken PHPUnit test with notices enabled for AJAX fails on View with "AJAX: Yes" just got in so I believe there is another curlExec to be converted now (in core/modules/views/lib/Drupal/views/Tests/ViewAjaxTest.php)

Status:Needs work» Needs review
Issue tags:+Avoid commit conflicts
StatusFileSize
new4.31 KB
new810.1 KB
FAILED: [[SimpleTest]]: [MySQL] 58,724 pass(es), 4 fail(s), and 7 exception(s).
[ View ]

In just one day, there were more than 10 conflicts in this reroll, plus something like 10 new uses of "the old" drupalPost(). I don't want to be rerolling this endlessly.

Status:Needs review» Needs work

The last submitted patch, drupal_post_cleanup-2074037-35.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new850 bytes
new810.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,751 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Back to RTBC

StatusFileSize
new791.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,861 pass(es).
[ View ]

Reroll.

Title:Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requestsChange notice: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests
Priority:Normal» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committing it while it's hot!!

Committed and pushed to 8.x. Thanks!

Let's get a very fast turnaround on the change notice for this, please. It affects a ton of people/patches.

Status:Active» Needs review

Proposal for review

drupalPost() and drupalPostAJAX() have been renamed

Summary:

  • In Drupal7, drupalPost() and drupalPostAJAX() both have deeply embedded assumptions that they're being applied to forms.
  • In Drupal 8, when testing e.g. rendering of contextual links, retrieving Edit module's metadata, etc. there is a need to simulate HTTP POST requests that do not have necessarily to be processed by a form controller.
  • A new drupalPost() method, with a different signature than Drupal 7 one, has been introduced in Drupal 8 to deal with these requirements.
  • The former drupalPost() and drupalPostAJAX() methods have been renamed, while retaining the original arguments:
    • drupalPost() => drupalPostForm()
    • drupalPostAJAX() => drupalPostAjaxForm()

Code example:

Before (D7):

<?php
  
...
  
$this->drupalPost('admin/config/development/logging', $edit, t('Save configuration'));
   ...
?>

After (D8):

<?php
  
...
 
$this->drupalPostForm('admin/config/development/logging', $edit, t('Save configuration'));
   ...
?>

+1 for the proposal.

Impressive piece of work.

Status:Needs review» Fixed
Issue tags:-Needs change record, -sprint

Wow, I didn't realize yet this was committed, or I'd have jumped on creating this change notice right this morning. Much thanks to mondrake for providing the proposal, which I've only expanded a little bit!

https://drupal.org/node/2087433

Title:Change notice: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requestsAdd drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests

.

I'd suggest a follow-up, please see #2087637: Add a drupalProcessAjaxResponse() method

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