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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake’s picture

Wim Leers’s picture

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

Wim Leers’s picture

Status: Active » Needs review
Issue tags: +sprint
FileSize
23.62 KB

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.

mondrake’s picture

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?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.58 KB
27.14 KB

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

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

Wim Leers’s picture

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

mondrake’s picture

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.

dawehner’s picture

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!

Wim Leers’s picture

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

#9:

  1. Hah, good point :) Fixed!
  2. :)
mondrake’s picture

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?

Wim Leers’s picture

#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? :)

mondrake’s picture

#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?

dawehner’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I wanted to RTBC that.

webchick’s picture

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.

mondrake’s picture

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

alexpott’s picture

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

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

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
11.29 KB
27.26 KB

s/drupalPostCustom(/drupalPostUrl(

Status: Reviewed & tested by the community » Needs work

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

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.37 KB
28.99 KB

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

Wim Leers’s picture

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

Updating title :)

Wim Leers’s picture

Dries’s picture

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

Wim Leers’s picture

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

webchick’s picture

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.

mondrake’s picture

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

Wim Leers’s picture

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

webchick’s picture

Make it so!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
792.6 KB
807.32 KB

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.

Wim Leers’s picture

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

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)

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Avoid commit conflicts
FileSize
4.31 KB
810.1 KB

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
850 bytes
810.24 KB
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

effulgentsia’s picture

Reroll.

webchick’s picture

Title: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests » Change 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.

mondrake’s picture

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

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

After (D8):

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


dawehner’s picture

+1 for the proposal.

Impressive piece of work.

Wim Leers’s picture

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

Wim Leers’s picture

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

.

mondrake’s picture

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

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