Problem/Motivation

Follow-up to #2263569-263: Bypass form caching by default for forms using #ajax.
Steps to reproduce
On drupal frash install.
Convert auto tagging widget on article to autocomplete
Click add more item button multiple time
Check request query
See the screenshot

Proposed resolution

Unknown

Remaining tasks

TBD

User interface changes

None

API changes

Unknown

Data model changes

Unknown

Comments

effulgentsia’s picture

I think the problem is in ajax.js:

ajax.options.url += Drupal.ajax.WRAPPER_FORMAT + '=drupal_' + (element_settings.dialogType || 'ajax');

That runs without checking if ajax.options.url already has that query param in it.

bruvers’s picture

Status: Active » Needs review
FileSize
1.18 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,849 pass(es). View

The submitted patch checks if the URL already contains FORMAT_WRAPPER and does not append it a second time.

Status: Needs review » Needs work

The last submitted patch, 2: ajax-check_url_for_FORMAT_WRAPPER-2508796-2.patch, failed testing.

Status: Needs work » Needs review
jaxxed’s picture

@bruvers fix looks to have fixed the repeated URL argument.

Attached is a screenshot of traffic captured following the instructions provided in the issue.

Notes:
- originally I wondered why the code didn't replace existing URL values with subsequent values, but @bruvers fix is more elegant, and uses a "first value passed", instead of "last value passed" approach.
- note that there are > 100 coding standards issues with the views module (out of scope for this issue) according to the coder project.

jaxxed’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Assigned: Unassigned » nod_
Status: Reviewed & tested by the community » Needs review
Issue tags: +JavaScript

Would be good to get nod_ to take a gander at this, since it affects JS.

nod_’s picture

My brain is wrecked right now but it seems possible that the format would change after a request. It means we do not only have to detect the presence of the variable but also it's value.

bruvers’s picture

FileSize
1.4 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,712 pass(es). View

Updated the patch. It now replaces the value of FORMAT_WRAPPER if it has been set earlier instead of just leaving it unchanged.

bruvers’s picture

FileSize
1.41 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,907 pass(es). View

Same patch as #9, just changed variable name for consistency with comments.

jaxxed’s picture

Status: Needs review » Reviewed & tested by the community

confirmed testing process. Functionality still behaves as expected.

I am also not aware of any case where the wrapper format changes, but the code should still work.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/misc/ajax.js
@@ -427,15 +427,19 @@
+      var regexPattern = new RegExp(Drupal.ajax.WRAPPER_FORMAT + '=\\w*');

Are we sure that this regex is good enough? Don't we really mean select up the next & or the end? Yes atm wrapper formats don't contain odd characters but it's not hard to imaging a - getting into one.

bruvers’s picture

Assigned: nod_ » Unassigned
Status: Needs work » Needs review
FileSize
460 bytes
1.41 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,031 pass(es). View

The new patch improves the regular expression: ignores case and the parameter value now matches any character except the ampersand "&". This should cover any special characters that could appear in the URL.

gints.erglis’s picture

Tested patch from #13 , it solved the issue.

gints.erglis’s picture

There are screenshots with request strings.

gints.erglis’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: core-check_query_for_FORMAT_WRAPPER-2508796-13.patch, failed testing.

droplet’s picture

  1. +++ b/core/misc/ajax.js
    @@ -427,15 +427,19 @@
    +    if (ajax.options.url.indexOf(Drupal.ajax.WRAPPER_FORMAT) === -1) {
    

    this condition is also case sensitive.

  2. +++ b/core/misc/ajax.js
    @@ -427,15 +427,19 @@
    +      var regexPattern = new RegExp(Drupal.ajax.WRAPPER_FORMAT + '=[^&]*', 'i');
    

    shouldn't it missing g flag? To clean them all in edge cases.

  3. +++ b/core/misc/ajax.js
    @@ -427,15 +427,19 @@
    +      var regexPattern = new RegExp(Drupal.ajax.WRAPPER_FORMAT + '=[^&]*', 'i');
    

    although it works, I think it still worth to add hardcoded `drupal_` namespace to regex.

  4. var WRAPPER_FORMAT = '_wrapper_format';
    var regexPattern = new RegExp( WRAPPER_FORMAT + '=[^&]*', 'i');
    "test?should_not_replaced_this_wrapper_format=z".replace(regexPattern, '!!!OUCH!!!');
    

    additional to above point, it's rarely but may occur.

jibran’s picture

Issue tags: +Needs reroll
deepakaryan1988’s picture

Assigned: Unassigned » deepakaryan1988
deepakaryan1988’s picture

deepakaryan1988’s picture

Assigned: deepakaryan1988 » Unassigned
Status: Needs work » Needs review
ajalan065’s picture

This patch fine for me..
+1

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.

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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.

Manuel Garcia’s picture

Issue tags: -Needs reroll

Patch on #22 still applies fine.

Fabianx’s picture

This is blocked on tests.

Manuel Garcia’s picture

Issue tags: +Needs JS testing

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.