The following line is a bit problematic.
$ajax_result = $this->drupalGetAJAX('ajax-test/dialog-contents', array(), array('Accept: application/vnd.drupal-modal'));
The problem is that this ends up sending the following headers:
Accept: application/vnd.drupal-dialog
Accept: application/vnd.drupal-ajax
nginx and apache are handling this differently. In apache PHP gets this for its header:
["HTTP_ACCEPT"]=> string(57) "application/vnd.drupal-modal, application/vnd.drupal-ajax"
but in nginx we're seeing this:
["HTTP_ACCEPT"]=> string(27) "application/vnd.drupal-ajax"
By only receiving the second header the test isn't behaving correctly because the server is negotiating the content type differently.
Adjust the "getAjax*" simpletest methods to detect when tests are supplying a different accept header and skip adding the default.
None
None
| Issue category | Bug because creates false failure in nginx testing environment |
|---|---|
| Issue priority | Normal bug outside our normal testing infrastructure |
| Unfrozen changes | Unfrozen because it only affects tests |
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | fix_dialogtest_outside-2467101-10.patch | 1.2 KB | neclimdul |
| #10 | mergediff.txt | 809 bytes | neclimdul |
Comments
Comment #1
neclimdulComment #2
fabianx commentedRTBC, looks great
Comment #3
wim leersRTBC +1
Comment #4
webchickI was trying to do some digging to figure out where these strange lines even came from, but unfortunately they're lost to the annals of time. https://github.com/drupal/drupal/commit/d532f23989860290afb72a6a18f0f370... was where it was most recently touched, but that was just a small tweak to https://github.com/drupal/drupal/commit/08c2cddea2b0732bdad9974052cf7ea7... which came from some bulk-merge, probably of WSCCI stuff.
Given this is test-only code the small behaviour change here is probably fine.
As a side note:
drupalGetAJAX / drupalPostAjaxForm
That makes me want to burn things. :P #2467887: Rename drupalGetAJAX to drupalGetAjax for parity with drupalPostAjaxForm
I would commit this, but jthorson is doing some hairy DrupalCI surgery atm so keeping HEAD stable atm.
Comment #5
alexpottShouldn't we be adding to the accept header if it already exists?
Comment #6
neclimdulMaybe? The expected behaviour is not defined so its what we decide.
The way I see this the two sides are
1) Current the behaviour we are testing is the combined header so we should preserve that.
2) It doesn't seem like testing combined headers was intentional and it isn't documented as the way the method works so we should avoid magic.
The reason I chose the later behaviour was it forces the asserting to code to clearly define the expected test rather then relying on understanding the implementation of the asserting. That seems more clean and I personally think that is the correct approach.
Comment #7
neclimdulWhat's the path forward on this? I think the patch behavior is correct but I'm not sure if we need to change it.
Comment #8
fabianx commentedIMHO if we set the header directly we don't want to append the other header as that is an internal default header ...
Or otherwise said:
- If the header is always appended I cannot test with only my added header => less flexible
- If the header is only added if there is no header, I can test with my only header or with both headers => more flexible
Therefore:
This patch makes the helper method more flexible - with minimal disruption.
Comment #10
neclimdulwhat fabianx said.
conflict: #2467887: Rename drupalGetAJAX to drupalGetAjax for parity with drupalPostAjaxForm
should be no change in patch.
Comment #11
neclimdulkick it off to testbot.
Comment #12
fabianx commentedRTBC if tests pass
Comment #13
alexpottFair enough. Committed 0bcac2f and pushed to 8.0.x. Thanks!