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:
WebTestBase::drupalPostCustom()
WebTestBase::getAjaxPageStatePostData()
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#39 | drupal_post_cleanup-2074037-39.patch | 791.79 KB | effulgentsia |
#37 | drupal_post_cleanup-2074037-37.patch | 810.24 KB | Wim Leers |
#37 | interdiff.txt | 850 bytes | Wim Leers |
#35 | drupal_post_cleanup-2074037-35.patch | 810.1 KB | Wim Leers |
#35 | interdiff.txt | 4.31 KB | Wim Leers |
Comments
Comment #1
mondrake+1
Found this problem also in
#2053519: Broken PHPUnit test with notices enabled for AJAX fails on View with "AJAX: Yes"
#2048309: Views UI Preview - navigation is broken
Comment #2
Wim LeersOh, 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!)
Comment #3
Wim LeersAnd 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 todrupalPostCustom()
.Nine instances of
curlExec()
were replaced withdrupalPostCustom()
.Comment #5
mondrakeajax.js does this:
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
by making the $post default to
$post = array('js' => 'true');
and so have an aligned js and simpletest behavior?
Comment #6
Wim LeersI'd forgotten to pull in the latest. Hence the patch didn't apply.
This reroll updates a newly added instance of
curlExec()
.Comment #7
Wim Leers#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.
Comment #8
mondrake#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.
Comment #9
dawehnerNice!!
Technically they don't need to be public (you just subclass anyway), but I don't care.
Way better!
Comment #10
Wim Leers#8: shouldn't that look at the
Accept
header insted?#9:
Comment #11
mondrakeRe. #8, I git blamed that file and found that
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
is dead, shouldn't it be removed first?
Comment #12
Wim Leers#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 ingetAjaxPageStatePostData()
, because then the function name is no longer accurate. Futhermore, it's entirely possible to useDrupal.ajax
andjs = true
not getting sent to the server (e.g. Edit.module's "attachments" call, which usesDrupal.ajax
too). IMO this should be fixed indrupalPostAJAX()
, 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 callingdrupalPostCustom()
.I think this is ready to go? :)
Comment #13
mondrake#12
That's fine.
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.
Fine for me, maybe more reviewers?
Comment #14
dawehnerI was wondering what this nocssjs is about, but this just got moved outside of this additional helper methods.
Comment #15
dawehnerI wanted to RTBC that.
Comment #16
webchickWell 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.
Comment #17
mondrakeIt's a wrapper around curlExec, so how about drupalPostExec()?
Comment #18
alexpottNeeds a reroll
Comment #19
dawehnerdrupalPostExec sadly does not tell you anything about the difference between the normal drupalPost. I really like the idea of webchick!
Comment #20
Wim Leerss/
drupalPostCustom(
/drupalPostUrl(
Comment #22
Wim Leers#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.
Comment #23
Wim LeersUpdating title :)
Comment #24
Wim LeersNote that this blocks #2040021: In-place editing of nodes does not create new revisions (when the content type is configured to create new revisions by default).
Comment #25
Dries CreditAttribution: Dries commentedIt's not clear from the method declaration what
drupalPostUrl()
does. It sounds like it is a more specialized function compared todrupalPost()
but it is really the more basic function. Would it not make more sense to rename the currentdrupalPost()
todrupalPostForm()
, and to makedrupalPostUrl()
drupalPost()
? Probably too big of a change ...Comment #26
Wim Leers#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.
Comment #27
webchickOk, 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.
Comment #28
mondrakeLet me then suggest to touch also the current 'drupalPostAJAX' then, and have 'form' in its name as well...
Comment #29
Wim LeersYes, that should then become
drupalPostAjaxForm()
. I'd like sign-off on that too from the core committers before working on this.Comment #30
webchickMake it so!
Comment #31
Wim LeersDone.
Definitely the largest interdiff I've ever posted!
Comment #33
Wim Leers#31: drupal_post_custom-2074037-31.patch queued for re-testing.
Comment #34
mondrake#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)
Comment #35
Wim LeersIn 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.Comment #37
Wim LeersI made a mistake in updating #34's #2053519: Broken PHPUnit test with notices enabled for AJAX fails on View with "AJAX: Yes" test.
Comment #38
mondrakeBack to RTBC
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedReroll.
Comment #40
webchickCommitting 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.
Comment #41
mondrakeProposal for review
drupalPost() and drupalPostAJAX() have been renamed
Summary:
Code example:
Before (D7):
After (D8):
Comment #42
dawehner+1 for the proposal.
Impressive piece of work.
Comment #43
Wim LeersWow, 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
Comment #44
Wim Leers.
Comment #45
mondrakeI'd suggest a follow-up, please see #2087637: Add a drupalProcessAjaxResponse() method