Currently when form has file element there's no ability to send ajax_html_ids
because it's not possible to serialize that array to send with jQuery Form plugin shipped with core.
Also ajax_html_ids
has array structure that makes ajax requests take more bandwidth by prefixing each value with
ajax_html_ids[]
.
Proposed solution
Send ajax_html_ids
serialized imploded with space because HTML5 standard does not limits allowed characters for HTML IDs except 'white-space'.
The ajax_page_state
already has plain structure so this is some kind of standardization for ajax variables.
Also this change help to minimize request size for ajax calls as discussed in #956186: Allow AJAX to use GET requests
Testing troubles
Currently simpletest has no ability to test ajax request properly so only manual testing possible.
Steps to reproduce
- download and enable attached module (ajaxdemo)
- visit /ajaxdemo_test_html_ids and press Get
-- bellow will output count and list of used IDs (they are passed as array)
- visit ajaxdemo_test_html_ids/1 and press Get (file element should appear on form)
-- there would be only one element serialized with ',' (in this case core can't extract IDs used on page)
Related #1574560: Update jQuery Form Plugin to the latest in the git repository
Comments
Comment #1
andypostProposed patch
Also attached a module to test
Screens of module
-- without file element $ids are passed as array
-- with file element $ids are broken
Comment #2
andypostTagging, suppose this does not break any existing code so could be backported
Also there's a lot of troubles could be fixed with this small change, forums has a lot of posts when ajax calls print garbage on screen.
PS: found when working on #1414510-41: Remove drupal_reset_static for drupal_html_id() when form validation fails
Comment #3
nod_+1 much better. good to go for me.
I'll let the ajax maintainers have a look.
Comment #4
andypostOn vanila D8 install data for ajax_html_ids reduced twice
befora patch
ajax_html_ids%5B%5D=overlay-profile-link&ajax_html_ids%5B%5D=overlay-dismiss-message&ajax_html_ids%5B%5D=toolbar&ajax_html_ids%5B%5D=toolbar-home&ajax_html_ids%5B%5D=toolbar-user&ajax_html_ids%5B%5D=toolbar-menu&ajax_html_ids%5B%5D=toolbar-link-admin-dashboard&ajax_html_ids%5B%5D=toolbar-link-admin-content&ajax_html_ids%5B%5D=toolbar-link-admin-structure&ajax_html_ids%5B%5D=toolbar-link-admin-appearance&ajax_html_ids%5B%5D=toolbar-link-admin-people&ajax_html_ids%5B%5D=toolbar-link-admin-modules&ajax_html_ids%5B%5D=toolbar-link-admin-config&ajax_html_ids%5B%5D=toolbar-link-admin-reports&ajax_html_ids%5B%5D=toolbar-link-admin-help&ajax_html_ids%5B%5D=shortcut-toolbar&ajax_html_ids%5B%5D=edit-shortcuts&ajax_html_ids%5B%5D=page-wrapper&ajax_html_ids%5B%5D=page&ajax_html_ids%5B%5D=header&ajax_html_ids%5B%5D=logo&ajax_html_ids%5B%5D=name-and-slogan&ajax_html_ids%5B%5D=site-name&ajax_html_ids%5B%5D=main-menu&ajax_html_ids%5B%5D=main-menu-links&ajax_html_ids%5B%5D=secondary-menu&ajax_html_ids%5B%5D=secondary-menu-links&ajax_html_ids%5B%5D=main-wrapper&ajax_html_ids%5B%5D=main&ajax_html_ids%5B%5D=sidebar-first&ajax_html_ids%5B%5D=block-search-form&ajax_html_ids%5B%5D=search-block-form&ajax_html_ids%5B%5D=edit-search-block-form--2&ajax_html_ids%5B%5D=edit-actions&ajax_html_ids%5B%5D=edit-submit--2&ajax_html_ids%5B%5D=block-system-navigation&ajax_html_ids%5B%5D=content&ajax_html_ids%5B%5D=main-content&ajax_html_ids%5B%5D=page-title&ajax_html_ids%5B%5D=block-system-main&ajax_html_ids%5B%5D=ajaxdemo-html-ids-form&ajax_html_ids%5B%5D=edit-submit&ajax_html_ids%5B%5D=message_area&ajax_html_ids%5B%5D=footer-wrapper&ajax_html_ids%5B%5D=footer&ajax_html_ids%5B%5D=block-system-powered-by
after patch
ajax_html_ids=skip-link+overlay-disable-message+overlay-profile-link+overlay-dismiss-message+toolbar+toolbar-home+toolbar-user+toolbar-menu+toolbar-link-admin-dashboard+toolbar-link-admin-content+toolbar-link-admin-structure+toolbar-link-admin-appearance+toolbar-link-admin-people+toolbar-link-admin-modules+toolbar-link-admin-config+toolbar-link-admin-reports+toolbar-link-admin-help+shortcut-toolbar+edit-shortcuts+page-wrapper+page+header+logo+name-and-slogan+site-name+main-menu+main-menu-links+secondary-menu+secondary-menu-links+main-wrapper+main+sidebar-first+block-search-form+search-block-form+edit-search-block-form--2+edit-actions+edit-submit--2+block-system-navigation+content+main-content+page-title+block-system-main+ajaxdemo-html-ids-form+edit-submit+message_area+footer-wrapper+footer+block-system-powered-by
Comment #5
sunFor file uploads, jQuery Form and our Ajax framework are using an iframe containing a textarea element that contains the embedded JSON returned from the server.
See http://api.drupal.org/api/drupal/core!includes!ajax.inc/function/ajax_de...
If the HTML IDs are broken, then other returned JSON values are possibly broken, too. We should check for potential JSON encoding and decoding differences instead.
Updating jquery.form.js to a later version might also resolve the issue.
Comment #6
andypost@sun this issue about frontend and how jQuery Form serializes data before send (see ajax.js# Drupal.ajax.prototype.beforeSend )
As I written
ajax_html_ids
does not send to Drupal when form hasencoding=multipart/form-date
I leave a performance tag for reasons of optimization POSTed data
There's already #1574560: Update jQuery Form Plugin to the latest in the git repository
EDIT if jQueryForm get updated to latest version (which uses JS strict) then ajax submission form with multipard encoding would fail with error:
An error occurred while attempting to process /system/ajax: Component returned failure code: 0x80460001 (NS_ERROR_CANNOT_CONVERT_DATA) [nsIDOMFormData.append]
Comment #7
andypostre-titled
Comment #8
andypostre-roll chasing core
Comment #9
cweagansFixing tags per http://drupal.org/node/1517250
Comment #10
attiks CreditAttribution: attiks commentedRegarding #6, there's a pull request at https://github.com/malsup/form/pull/201/files
Comment #11
andypostnot sure it's possible to backport to D7 as is but we can make a check to make sure that html_ids are passed as array and in cahe of string just decode them back to array
Comment #12
andypost#8: 1575060-htmlid-8.patch queued for re-testing.
Comment #13
mcjim CreditAttribution: mcjim commentedRe-roll to chase core.
Comment #14
andypost#13: 1575060-htmlid-13.patch queued for re-testing.
Comment #15
andypostLet's get this commited before feature freeze
Comment #16
nod_Can we reroll using querySelectorAll please?
35% faster is kinda significant since we have so many ids on a page usually.
http://jsperf.com/drupal-id-selector/2
(edit) Turns out there was a problem in my test, andypost fixed it, thanks :) new link added.
Comment #17
andypostRe-roll for qSA a lot of tests could be found in http://jsperf.com/drupal-id-selector/2
Comment #18
nod_tested and works very well :)
I changed the way we access
ajax_html_ids
, JSHint was complaining. I wasn't too happy about changing the type ofajax_html_ids
so I just used another variable, no functional change, just moving things around :)Comment #19
nod_forgot tag
Comment #20
andypostGreat! This should be a bit faster then access to array element
Comment #21
sunI guess it's ok to go with this for now (since we need to backport it), but we should really try to get the PR in #10 in the upstream library, so that we can remove this workaround.
That is, because this workaround is a one-off solution for particular extra data we're sending, and it's perfectly possible that there are contrib and custom modules affected by this jQuery Form bug, too.
However, one last nit: The variable dump in #1 showed commas as separators, whereas the latest patch uses spaces. The HTML spec does not allow for commas in IDs, and passing a list using commas as delimiter appears much more natural to me (and also avoids URI escaping of spaces), so I wonder whether we could change the delimiter back to commas?
Comment #22
nod_@sun, about the space, thought that as well. Turns out, ",", ":" and some other weird characters are allowed in ids. Only the space is explicitly forbidden by the spec. http://www.w3.org/TR/html-markup/global-attributes.html#common.attrs.id
The nice benefit is how much the request size shrinks too.
Comment #23
sundrupal_html_id() documents
- http://www.w3.org/TR/html4/types.html#type-name
- http://www.w3.org/TR/CSS21/syndata.html#characters
as the main constraints for HTML IDs. HTML5 is less restrictive, and this use-case is different, so hm, it's probably fine to diverge from drupal_html_id(). (though I wonder whether it wouldn't be helpful for future reference and readers of ajax.js to have an inline comment in there similar to the one in drupal_html_id() that explains the situation)
Comment #24
webchickWe talked at length in IRC about large refactoring patches like this, and their likelihood to break things in core. We decided to tag issues like this with "Needs JS testing" and then zendoodles and her Merry Band of Contributors will test these patches manually (for now) and with automated testing (later!).
Tagging so she can find it. :)
Comment #25
attiks CreditAttribution: attiks commented#10 just got fixed upstream
Comment #26
attiks CreditAttribution: attiks commentedNew version contains the fix, see http://malsup.github.com/jquery.form.js
Comment #27
kbasarab CreditAttribution: kbasarab commentedFollowed instructions in issue summary for manual testing. I'm getting same results with patch applied and not applied:
See screenshots attached.
Comment #28
Kjartan CreditAttribution: Kjartan commentedAfter checking up on this issue I can confirm that the behaviour described in the issue summary is correct, and that the patch works as intended.
I did additional testing on AJAX requests in Core and couldn't find anything broken by this patch. I couldn't find anything special in core that seems to rely on $_POST['ajax_html_ids'] that would be impacted by this patch.
Comment #29
catchThanks for the manual testing. Committed/pushed to 8.x.
Comment #30
andypostFor D7 we can't use
document.querySelectorAll()
so patch should use fastest approach from testing #17,also we should file a change-notice about browser compatibility and new format of
ajax-html-ids
Comment #31
serm CreditAttribution: serm commentedRe-roll for D7
Comment #32
andypostComment #33
serm CreditAttribution: serm commentedSorry. Added simpletest to patch.
Comment #34
andypostWe need David Rothstein confirmation on new format of data, or we just proceed with tremendous array for D7
Comment #35
webchickMaking it official. :)
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedIf this comment from #11 is correct:
then that sounds like a much better way to fix this issue in Drupal 7 to me, rather than changing $_POST['ajax_html_ids'] for everyone.
For example, I just did a grep through a large codebase of an actual Drupal 7 site and found all sorts of modules doing things with $_POST['ajax_html_ids'] directly. These included Panels, Ctools, Views, etc. Although most didn't look like they were doing much with it except forcefully unsetting it (or resetting it to an empty array) in certain conditions, and I didn't look too far in depth into what they were doing, that's enough to give me pause about the idea of changing this from an array to a string in the middle of a stable release, especially if we don't have to.
Comment #37
andypostJust wanna to mention about unset used in contrib - this all caused by 2 bugs in core - this one and #1414510: Remove drupal_reset_static for drupal_html_id() when form validation fails
So contrib modules can't use this data because it could be easily broken (element IDs are changing) so this leads to #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests
For D7 I see only one solution - implement a check for
$_POST['ajax_html_ids']
that it is a string or array and in case on string make it arrayComment #38
andypostMarked as duplicate #1811100: File field AJAX wrapper always targets first instance on the page
Comment #39
sonictruth CreditAttribution: sonictruth commentedI came across this bug when building a module that enables you to create entities via ajax. The #33 D7 patch seems to fix the problem for me. Thanks guys.
Comment #40
jeff h CreditAttribution: jeff h commentedI can confirm that the patch in #33 works and is badly needed when doing complex #ajax stuff. Let's please get this in 7 core.
Comment #41
SpleshkaI think we can simply check recieved data and convert it into array if we got string.
Patch attached.
Comment #43
SpleshkaOps, my bad. New patch attached.
Comment #44
andypostBecause the contrib could have own fixes for the issue we should accept #43
While I think the the proper fix is #33 but a bit progressive as David pointed in #36 most of contrib just
unset($_POST['ajax_html_ids'])
to recalculate IDsComment #45
sonictruth CreditAttribution: sonictruth commentedThis suits me—as long as those ids get converted to an array I'm happy and our module will be actually usable: Entity Dialog
Comment #46
andypostClosely related issue #799356: _form_set_class() is too aggressive in assigning the 'error' class needs to be commited after this change
the line exceeds 80 chars, so here's re-roll
Comment #47
SpleshkaThanks @andypost!
Re-rolled patch with more clear comment.
Comment #48
andypostNot sure that tests should work with wrong format. Let's revert this part or add a condition
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedAgreed, I don't think we should make that kind of change to a fundamental test method like drupalPostAJAX() as part of this patch... that should really use the expected format. So, "needs work"?
(I'm not sure if it's worth a D7 test or not though, but if so we should use a separate method or do it with a conditional parameter as @andypost suggested.)
Also while we're at it:
Those "specific reasons" aren't very specific :) How about:
Comment #50
SpleshkaRe-rolled patch with new comments.
Comment #51
andypostsuppose this part better to drop for D7.
Then we need to revisit this for D8 once the issue no more reproducible after upgrade of form library
Comment #52
SpleshkaDroped test part.
Comment #53
andypostJust removed simpletest hunk
For D7 we just need to fix bug
Sorry for crosspost
Comment #55
andypostMagically stopping the bot breaks status
Comment #56
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/005708c
Comment #56.0
andypostAdded ref to html5 specs
Comment #57
andypostAccording #51, #21 we need to update D8 library to fixed version with commit https://github.com/malsup/form/pull/201/files
Also issue needs change notice about D8 ajax_html_id which now a string
Related issue added #1574560: Update jQuery Form Plugin to the latest in the git repository
Comment #58
SebCorbin CreditAttribution: SebCorbin commentedThe fix in #56 did not resolve the issue. When I download and install ajaxdemo from #1 and test it with Drupal 7, I get
The fix in #56 only works when
$_POST[ajax_html_ids] = 'skip-link,toolbar, ... '
which is not the caseAttached is a patch that rectify this. Again, this can only be manually tested.
Comment #59
andypostTested with module #1
=== FALSE - means no comma found
Comment #60
SebCorbin CreditAttribution: SebCorbin commentedRight, sorry for the mistake on the condition
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedOK, thanks for the extra testing. Hopefully this time fixes it for real. Committed to 7.x (with a small fix to grammar in the code comment) - thanks! http://drupalcode.org/project/drupal.git/commit/d6f960c
Back to Drupal 8, per #57.
Comment #62
andypostThanx, David!!!
We can close the issue in favour of #1574560: Update jQuery Form Plugin to the latest in the git repository and #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests
Last one should file change notice when the route would be chosen
From IRC:
<nod_> andypost: is updating the lib fixes the issue, sure I'm ok with it.
Comment #63.0
(not verified) CreditAttribution: commentedUpdated issue summary.