Proposed commit message:
Issue #2502785 by dawehner, effulgentsia, tim.plunkett, amateescu, Wim Leers, Fabianx, catch, dsnopek, EclipseGc, yched, mondrake, larowlan, olli, Berdir: Remove support for $form_state->setCached() for GET requests
Core committers can obviously decide differently, but I read every comment and all those made useful contributions to the patch / approach.
Problem/Motivation
- Calling
$form_state->setCached()
during a form's initial build (the GET request) leads to several problems:- Though named
setCached()
, what is actually persisted is state, and changing state during a GET request is something that the HTTP spec says SHOULD NOT be done. - When a page containing such a form is cached, then 2 different anonymous users end up submitting forms with the same build ID, and special care is required to prevent information disclosure between them. SA-CORE-2014-002 was required to fix this for D6 and D7, and #2421503: SA-CORE-2014-002 forward port only checks internal cache remains an open critical issue to complete the fix for D8.
- When $form_state contains an entity UUID for a creation form and is shared between multiple users (see above), submissions by all users after the first one result in fatal errors due to UUID collision on insert. #2465053: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state is an open critical issue for the user registration form having that problem.
- For uncached pages (e.g., for authenticated users), every page view (e.g., refresh) results in a new form build and corresponding $form/$form_state entries persisted, leading to scalability problems for the corresponding storage. For example, see #2252763: Views exposed filter form causes enormous form state cache entries.
- Though named
- Form elements that implement a custom
#ajax['url']
with a route controller that extendsFormAjaxController
rely on $form_state caching/persistence on the GET request, because that controller can't rebuild the form reliably and securely. - After #2263569: Bypass form caching by default for forms using #ajax., #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache, and #2500523: Rewrite views_ui_add_ajax_trigger() to not rely on /system/ajax., D8 core will no longer have any use cases for
FormAjaxController
or any other$form_state->setCached()
on GET requests. - However, contrib still does. #2263569-127: Bypass form caching by default for forms using #ajax. links to 3 such modules and there might be others. I checked Field Collection and Hierarchical Select and believe those to be easily convertible to
#ajax['callback']
. But the Panopoly use case might be trickier. Also, Mollom doesn't use#ajax['url']
, but does call$form_state->setCached()
to maintain CAPTCHA state between the GET and POST. - So, should we remove this support and force such contrib use cases to refactor to work better with HTTP?
Proposed resolution
Yes. Remove this support. Keeping it is more trouble than it's worth.
Remaining tasks
Review. Commit.
User interface changes
None.
API changes
FormAjaxController
removed as a base class. Contrib #ajax['url'] controllers that extended this need to be refactored to use #ajax['callback'] instead.$form_state->setCached()
modified to throw an exception.
Comment | File | Size | Author |
---|---|---|---|
#125 | interdiff.txt | 1.52 KB | dawehner |
#125 | 2502785-125.patch | 51.48 KB | dawehner |
#116 | interdiff.txt | 1.96 KB | effulgentsia |
#116 | 2502785-116.patch | 51.34 KB | effulgentsia |
#113 | interdiff.txt | 8.86 KB | dawehner |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #2
catchAbsolutely agreed on removing support for this. That we had a Drupal 7 SA, and 3 out of 22 critical issues are related to this shows just how much trouble it is.
I do think we also need the 'fast path' follow-up from #2263569: Bypass form caching by default for forms using #ajax. for AJAX requests opened too though - so that it's possible to post to a separate URL again for AJAX requests (with the ability to rebuild the form reliably).
Comment #3
dsnopekWould it be fair to say that if we can implement this with
#ajax['callback']
in Drupal 7, that we'd be able to implement with#ajax['callback']
in Drupal 8?Panopoly's case is interesting, because what we're doing is auto submitting the form as the user is changing it (via the CTools auto submit functionality) and returning via AJAX the rendered version of the thing they're editing (a CTools content type plugin -- a "Panels block" for those who don't do lots of CTools/Panels) to provide live previews.
Using
#ajax['url']
makes sense from Panopoly's perspective because we're not returning any part of the form, we're just taking it's values and rendering the CTools content type. However, if we rendered the preview in the form constructor into a'#type' => 'markup'
element, then I think we could use#ajax['callback']
to just extract that element from the form.But I'd need to actually try to do it to see if it's possible or not!
The extra interesting thing is that we're not including the preview markup inside the
<form>
tag because it caused some sort of issues in the past, but looking at the issue now (#2017159: Panopoly Magic: render preview outside of pane configuration form), I don't entirely understand what the problem was...Anyway, if doing it in Drupal 7 would prove that it would work in Drupal 8, then I can do some experimentation!
Comment #4
tim.plunkettThe idea would be to try to accomplish everything with #ajax][callback, yes. You can get pretty creative with #ajax][wrapper too, targeting hidden divs or whatnot.
Comment #5
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#3: Interesting, indeed you can get very creative using a special "ID" wrapper and returning ajax commands that replace the "#my-special-preview-id".
That should work regardless of the form's state or maybe using wrapper might work directly.
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedSo, there are contrib needs to modify form_state during an ajax callback. I'm doing this in CTools 8.x-3.x right now by providing a different url route for wizard values during ajax. Looking at #2263569: Bypass form caching by default for forms using #ajax. it appears I'm going to need to switch to having a higher-priority event subscriber that tries to determine if this ajax request should be intercepted by it or let it fall through to core's. Any thoughts on that approach? Just making sure we give contrib proper consideration here.
Eclipse
Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#6: You can change $form_state all you want, you just cannot depend on any $form_state being set already on the first GET.
Comment #8
EclipseGc CreditAttribution: EclipseGc commentedI'm just saying I want a generic way to alter the form state before it is passed to the ajax callback method specified in the #ajax array.
Comment #9
Wim Leers+1, the issue summary is overwhelmingly convincing.
Comment #10
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedFor the record: We discussed this on the EU criticals call today and all participants have been in favor of removing it.
The main argument being:
If core can so easily run into it with _massive_ problems, then contrib will get the same problems and if D6/D7 history is any indication that will happen.
Therefore it is good to remove this before 8.0.0, so that no-one runs into that problem anymore. (and we are semantically correct not to write "state" changing data on a GET request)
Comment #11
effulgentsia CreditAttribution: effulgentsia at Acquia commentedGiven #10, removing the question mark from the title and postponing implementation on:
- #2263569: Bypass form caching by default for forms using #ajax. (close to done)
- #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache
- #2500523: Rewrite views_ui_add_ajax_trigger() to not rely on /system/ajax.
@dsnopek, @EclipseGc: thank you for commenting on your use cases. I still plan on responding to them. Other contrib maintainers: despite the postponed status of this issue, please also share your use cases if you have a concern, since we can keep that discussion going in the meantime. The only thing postponed here is the patch.
Comment #12
dsnopek@effulgentsia: I'm pretty sure Panopoly would be fine without
#ajax['url']
(but I do still plan to do some experiments just in case) so no need to spend too much time responding. :-)Comment #13
dsnopekI created a Panopoly issue for my experiments: #2505369: Experiment with not using #ajax['url'] - it's going away in Drupal 8 (and shouldn't be necessary)
Comment #14
effulgentsia CreditAttribution: effulgentsia at Acquia commented#2263569: Bypass form caching by default for forms using #ajax. down. 2 to go.
Comment #15
alexpott@catch, @effulgentsia, @webchick, @xjm and I discussed this issue and decided to keep it critical since we've made several errors through the d8 cycle because of this and not doing will make everything easier for contrib and custom. Also
is extremely compelling.
Comment #16
Wim Leers+1
This is a perfect example of where a decision made a long time ago for likely sensible reasons, but that didn't consider one small aspect (changing state during HTTP GET), causes enormous amounts of issues (in DX, scalability and cacheability).
Comment #17
dawehner#2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache landed
Comment #18
Wim LeersJust to be clear, this is now only blocked on #2500523: Rewrite views_ui_add_ajax_trigger() to not rely on /system/ajax., right?
Comment #19
catchI think so yeah. If we have a patch here to ditch it, then it should fail until that goes in, then pass.
Since that issue is RTBC, I think it's worth having that patch here whether the other one is in or not.
Comment #20
effulgentsia CreditAttribution: effulgentsia at Acquia commented#18 landed, so now fully unpostponed!
Comment #21
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSo, there are legitimate use cases for calling $form_state->setCached() on POST requests. For example, for a captcha module to put itself into a solved state if it was solved correctly, but the form has other unrelated validation errors, so needs to be redisplayed. But a hook_form_alter(), for example, runs on both the initial GET and again on the POST. Given that, would it be better for an exception to be thrown if called during a GET (requiring the module to have logic within hook_form_alter() to only call it during POSTs)? Or better to allow the method to be called regardless, but ignore it if it's a GET? Or, any other ideas?
Comment #22
tim.plunkettDepends on how loud we want to be about it.
Let's see what happens.
Comment #24
tim.plunkettWow, actually found a bug in FormState.
FormState::isMethodType() and FormState::setMethod() both use strtoupper for comparisons.
But the default value of FormState::$method is 'post'.
Comment #25
tim.plunkettThis combines the two green patches, and also tries to mangle RenderElement enough that no one can use #ajax][url anymore.
Comment #29
dawehnerI could not resist to try to debug some of the failures ... but there are still uses of #ajax][ in other places in the views UI.
Comment #31
dawehnerThis does not fixes all issues yet.
Comment #33
dawehnerWe still have to support to add custom query parameters.
Comment #35
tim.plunkettWe need to completely remove system.ajax here.
I was pretty confident with the changes, except those in drupalPostAjaxForm(). What do we fallback to here?
Comment #37
dawehnerWhat about ?
Questions we had during the talking:
#ajax => []
the expected behaviour, also UI wise?Comment #38
dawehnerMh, so in case you specify a callback, which renders the entire page the actual preview UI fails.
It posts to admin/structure/views/content for example. The form submission handling of the other form on the page is triggered
and you end up in
throw new HttpException(500, 'The specified #ajax callback is empty or not callable.');
Interesting though is that its not the main edit form, but rather the exposed form, which causes issues, see debugging screenshot.
Comment #39
dawehnerSo the problem described in #38 is as said related with exposed forms in the preview.
Let's assume that problem does not exist, there is a different problem. The pager URLs contain ?ajax_form=1 which is then interpreted as form submits by the system.
We can workaround that problem using a fix in pager.inc
Comment #40
tim.plunkettNeeds better text.
The
= NULL
is overkill now with that exception.AJAX
Not sure how we're getting around these empty strings now, but they will break if used with base://
Yay!
This seems odd having just
#ajax => []
(I guess that's why you switched to !isset above)What a hack! Needs docs if we're going to keep it.
Comment #41
dawehnerDid a little bit more of research.
I think something along those lines are needed in order to fix the exposed form in the views preview UI.
#40 is not adressed yet, as tim said he wants/promised/... to do it.
Comment #42
tim.plunkettYep, working on it now.
Comment #43
tim.plunkettThis breaks the Views UI visually for me, when triggering a pager during preview, the top part of the Views UI is duplicated.
The test coverage only uses the preview in isolation, not as a larger part of the form.
I addressed all of my own feedback from #40.
Comment #44
tim.plunkettYeah I can't reproduce this in a test, nor can I track down a fix. Here's a video.
Steps to reproduce:
Create 2 nodes
Go to admin/structure/views/view/content
Change the pager to show 1 item
Use the pager in the preview
Video:
https://www.drupal.org/files/issues/views_ui_fun.mp4
Comment #45
dawehnerYou know, you can never have enough views UI. It would be kinda a version history built into the interface, pretty neat.
Back to business, I had a look at why this is happening. The preview form itself,
is using the form rendering / ajax processing mechanism to render itself, so this means it just renders the form and then ajax.js replaces
#views-preview-wrapper
with the rendered preview which contains both the form again, but also the renderer view.
If we have a look at the different between the output of this patch vs. HEAD you can see one major difference. The pager URLs are different.
In HEAD we point to admin/structure/views/content/preview, with #43 we though point to admin/structure/views/content itself. So what happens is that the pager
itself renders the entire page, not just the views preview itself.
If we have a look at
\Drupal\views_ui\ViewUI::renderPreview
we are doing quite some work in order to mock a requestand point to the entity.view.preview route. This would theoretically render the pager URLs in the context of
admin/structure/views/content/preview
,too bad that it simply doesn't work. Let's fix that.
Not sure whether we can test this for real, but at least we should have a check that the links point to the right URL.
I also did a look at the interdiff in #43 and it looks alright for me
Yeah I agree, this is the less confusing API.
Comment #46
Wim LeersI reviewed the entire patch, without having been involved in this issue. Apologies if some things have been discussed earlier. I thought a fresh look would be useful.
These seem to contradict?
So the reason for
'page'
being excluded is crystal clear.The reason for the newly added exclusion is less clear. Perhaps add a line of documentation? Especially the fact that it's
\Drupal\Core\Form\FormBuilderInterface::AJAX_FORM_REQUEST
that we're excluding and not\Drupal\Core\EventSubscriber\MainContentViewSubscriber::WRAPPER_FORMAT
is not intuitively clear. At least not to me.Nit:
The comment is not clear, and I do not understand the
!$this->isSubmitted()
condition. I bet it's got something to do with how how AJAX GET forms are submitted, but without knowing the details of how that works, this does not seem understandable.So: a better comment would be most helpful :)
Oh hrm… so this seems to contradict the previous bit, because it seems to say that only POST forms can ever be cached?
Why not remove
processAjaxForm()
then, and just update all calls to callpreRenderAjaxForm()
instead?Or would that be a public API change/BC break?
This exists to aid the transition to D8, right? Perhaps add a @todo to remove that exception in D9?
Nit: s/still be given an event/automatically be assigned a sensible event/
An @see to point to the internals that use this would be helpful.
What happens if
callback
is not set? Given that neitherpath
norurl
are allowed, an initial reading would suggest that#ajax
is then effectively a no-op?Woah, this is some very confusing code!
Oh wow, I wonder how that works! I wonder if that change is actually necessary?
Wow, nice!
Just to check my understanding: this test is removed because it no longer makes sense, because we disallow forms from getting cached on the GET request, correct?
Here we also add
\Drupal\Core\Form\FormBuilderInterface::AJAX_FORM_REQUEST
. Probably the same explanation as the one above will apply.AFAICT this can be simplified to the following?
Nit: include the exception message — this makes it clearer which exact logic exception you're expecting here (and tests that too).
Nit: s/Get/GET/
Wait, what?! I didn't know we had this. Interesting :)
Comment #47
mondrake#46.2 - wouldn't #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs be a more robust solution here? Besides pager links other links may be generated in the preview (e.g. tablesort links) and still take on the carried over querystring elements
Comment #48
dawehnerPotentially yes, but I would like to see the critical fixed.
Mh I'm not sure what you mean. While forms now always post to the same URL, the ajax framework itself still allows you to use different URLs. Views is heavily using that for example.
Its just the fact that #ajax as part for FAPI no longer can, because that URL controller relied on form cache to exist. Other examples, like views, rely on the usage of tempstore and work fine in the way how it works.
Well, that is kinda the main question. How do we exactly treat GET form submissions.
This is what happening in HEAD already so yeah setCached() ideally should not be called I think.
Yeah
Either we keep it like that or we write an update function + update test as this missing function would break things, unless the cache is cleared.
Well, the more I think about it, I think having two distinct function, one as a preRender and one as a process function makes sense and its a pure implementation detail that internally they can reuse the code.
Well no, you can still render the entire form, see
#ajax => TRUE
Meh
Not sure, honestly.
Exactly.
Yeah it doesn't, but its documented below already.
Copy stuff but remove ajax-framework specific keys
I'd rather argue that we should have a dedicated exception type
Let's open up a technical working group issue first, oh damnit, too bad, we already use prophesizy in core, too bad that it is too late.
Comment #49
dawehnerImplemented Point 16.
Comment #50
dawehner.
Comment #51
tim.plunkettOnly responding to the parts I fully grok, the rest was @dawehner.
#46.4 I don't fully grok the addition of the isSubmitted() part as well, I'm not sure that's the right flag.
#46.5 Yes, this is now contradictory
#46.6 '#type' => 'link' cannot have a #process, it can only have #pre_render. Or is it the other way around? But yeah I think we still need this, effulgentsia knows.
#46.10 The 'event' processing will still happen. For example, if you now have
'#ajax' => TRUE
for a select element,['event' => 'change', 'dialogType' => 'ajax']
will be passed to drupalSettings for that element.#46.12 Well we had to change it to *something*, as we've removed /system/ajax
#46.14 Yes, exactly.
Comment #52
dawehnerYeah they are two separate concepts ...
When @amateesc, @wimleers and @plach talked about that, we thought that the logic place would be better in the form submission code.
Comment #53
Wim Leers#48.1/#46.1: I meant the fact that we remove the
'path'
documentation, yet later refer to it. Still a problem.#46.6:
-> I guess let's defer to a non-critical follow-up then. Let's add a @todo?#46.17:
Agreed, but I didn't want to impose more work on you :)The changes in #52 are awesome :)
Thanks for adding these docs to address #46.2. But it's not entirely clear to me yet.
s/of an ajax/of an AJAX/
s/from there/from the query arguments/
s/be interpreted potentially/be potentially interpreted/
s/as form/as an AJAX form/
Let's keep the outer comment, check the if-test to
not GET
, and remove the inner comment.Don't we now disallow caching of a GET form ever? If so, then both the comment and exception need to be updated. And I think the comment can in fact be removed: the exception alone is sufficiently clear.
Comment #54
dawehnerWell there is also the thing of #process vs. #pre_render
There is not potentiallity here, it happens.
Addressing the feedback from wim for now. Berdir feedback is coming directly afterwards.
Comment #55
BerdirAs discussed, empty string would be the frontpage, so that doesn't seem to make sense. It would have to either default to the current page or throw an exception if we know that it always exists.
That might also affect this, not exactly sure how, though.
This seems similar, no else anymore, so we should either throw an error if we can't find or rewrite this a bit so we just assume it's there?
Comment #56
Wim LeersNit: I think the comment can be removed, the exception alone is sufficient.
Comment #58
dawehnerIf I remember all the cases, there should be always a path, so let's see whether we fail ...
The failure was a checking to the exception message ...
Comment #59
Wim LeersDo we use trailing periods for exception messages?
Also: s/ajax/#ajax/ ?
:P
Comment #60
dawehner.
Comment #61
olli CreditAttribution: olli commentedThis does not work. If we dont set #ajax['url'], then changing formatter (when adding a field) will take you back to "Add fields" screen (similar problem as in #2248223: Adding a new Views filter and making it exposed returns user back to list of filters).
Nice.
unused?
I still don't fully understand why we need to completely remove support for #ajax['url'] even for cases like views ui.
Comment #62
dawehnerOh I've tried it manually, but you are right, the functionality is broken with that.
For views UI we don't rely on form caching but rather we use tempstore and create the form from that object, right.
Comment #63
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedOverall looks really great!
I think we should link to that other issue that provides methods to get the query parameters automatically with removed _format and ajax, etc.
Yes!
I assume we treat HEAD as GET requests. (not that it matters much here)
For those not knowing:
In $request there is a method, isMethodSafe() for that.
Is there a reason why we could not check $element['#ajax'] === TRUE
instead?
Oh, nice that means we can remove that now :)
Is this function still used?
Likely just needs a different callback instead.
This feels really good overall.
It simplifies our ajax system a lot.
Uh why is this needed and would that not always be $request that we get from the stack?
Actually that is wrong and the legacy render() function needs to be called, because else it would potentially be double rendered.
Another possibility is to use getCacheableRenderArray() here ...
Can be follow-up though ...
render() really is a lost child except for twig.
Comment #64
dawehner@olli
Working on it the entire morning .. the fields work fine for me, its rather the views UI which is broken, once you click preview twice... still try to find a proper solution.
Comment #65
Wim LeersMaybe this means we should change it back to
!POST
. Or just useisMethodSafe()
.Comment #66
dawehnerI tried to keep the old way, but I think we really don't need to. Do you mind providing the example which doesn't work? I manually tested it for quite a while and things continue to work, as expected, at least for me.
Yeah we have an issue to discuss this already: #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs
Ah good to remember.
I like that.
No we can't, views/ajax for example still exists ... but yeah I also made often the assumption that ajax.js === #ajax basically.
There is still some usage in
core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php:142
leftWell, we inject the $request into the view, ... this is a great DX thing in general, given that its add less "global"-ness but rather keeps things local. Nothing new though.
Fixed it.
Comment #67
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#66: What I meant was:
Why do we need to get it again from the request_stack when we just pushed $request to the stack, could we not use:
instead?
Comment #68
dawehnerWell, there are still things, like the pager, we call out to the request stack itself.
Comment #69
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSorry, I haven't been following the discussions since patching started in #22, but I read through the latest patch and it looks really great to me. I'll try to post some nits later today, but couldn't find anything significant to comment on, except:
I wonder if we should retain #ajax['url'] for #type => 'link'. I think an argument could be made not to, and that it's better for links to have their ajax submit to their href. But maybe making that choice should be a separate issue, since it's not really related to this one, which is about forms. Thoughts?
Comment #70
dawehnerThank you alex!
I agree, its a separate discussion needed. Also in general a custom URL is useful for other examples, for ones which don't care about caching, like views potentially which has everything in tempstore, so we can recover from that.
Comment #71
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#69 Links should be fine, unless we allow them to use a callback parameter.
The problems we had with the ajax system for posting to system/ajax were:
- Creating state on GET so that system/ajax knows the state
- Having a callback that had no access control or context of its own (only what system/ajax had)
So if those use neither I think we could be okay to allowing an ajax request to go to another URL - if it does not involve Form API.
Comment #72
dawehnerRight, we now don't cache on the GET request anymore, so people cannot rely on it any longer.
Comment #73
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI very much like the way Views UI was converted to not use #ajax['url'], but given #71, I agree with @olli and suggest punting that to a separate issue, and refocusing this issue on only $form_state->setCached().
Let's see if this works.
Comment #74
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedOpened #2527740: Consider to remove support for #ajax['url'] as follow-up, thanks #73!
Comment #75
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe patch looks ready to me. My changes in #73 was only about reverting the parts that are now in #2527740: Consider to remove support for #ajax['url'], so I'm tempted to RTBC, but will leave a bit longer at "needs review" to give people who want to comment on that a chance to do so.
Meanwhile, updated the IS for the scope reduction.
Comment #76
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe IS now lists 2 API changes:
Those are not covered by the current CR: https://www.drupal.org/node/2501435. That CR is shared with an earlier issue though, so I think maybe we want a new CR to cover the above two changes.
Comment #77
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNot quite. I also added the following docs, so highlighting it for review.
Comment #78
catchShould we also check HEAD too? We don't want those to write to cache either.
Also if we do check that, should we also check it when throwing the exception?
Looks great to me apart from that question.
Comment #79
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis fixes #78. It's also an improvement, because we need to check the request, not the $form_state method. In other words, we might be in a GET request that is building the form whose method attribute will be POST. In this case, the request method is GET, but the $form_state method is POST, so we should check the former, which this patch does.
Some unit tests probably need to be fixed accordingly.
Comment #80
effulgentsia CreditAttribution: effulgentsia at Acquia commentedahem
Comment #83
dawehnerTo be clear +1 to remove the decision whether to keep
#ajax][url
or not.Removing many test failures for now.
Actually TRACE and OPTIONS seems to be safe, but well, this is kinda an implementation detail.
Comment #84
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPer #79, this is wrong, because isMethodSafe() will return FALSE on e.g., the initial GET for /node/add/article (or any other regular form), because that form is considered a POST form (i.e., it will set the "method" attribute to POST) even during the GET request. So we need something more like isRequestMethodSafe() or else change $form_state->method to be the request's actual method rather than what the desired submission method is.
Comment #85
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis will probably fail the same tests as #80, but I believe is the correct way to address #84, so I think those tests need fixing. Or in some cases, the tests might be pointing to forms calling setCached() that shouldn't be.
Comment #87
yched CreditAttribution: yched commentedJust wondering : doesn't this potentially add to the dependency-chain hell when the form state does get stored/serialized ?
It seems earlier approaches didn't involve storing a ref to the Request in the FormState ?
Comment #88
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI agree with #87.
While I strongly believe in using a proper isMethodSafe() from request I think what would be fine is to use dawehners #83, but include a \Drupal::request()->isMethodSafe() check - which should be transparent.
Alternatively not the request should be injected, but however the getMethod() gets it information, should use the same for that.
Comment #89
dawehner.
Comment #90
dawehnerIt could be $form_state->updateFromRequest($request) to keep our API open for future internal changes, but for now we just store the method type.
Well yeah I also think we should avoid storing the request, unless we really have to.
Comment #91
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't think that $form_state ever gets serialized except via getCacheableArray(), which does not include 'request' or any other property whose docs say "This property is uncacheable.". FormBuilder::buildForm() calls ->setRequest() prior to retrieving the form/form-state from cache, which loads the cacheable portion of $form_state and merges it into what has already been prepared. Same concerns about what state can be persisted vs. not also applies to e.g., 'input'.
$form_state->updateFromRequest($request)
instead of ->setRequest() might be a good idea for other reasons though, in that the implementation would clarify which parts of $request are needed, while still leaving that decision with FormState rather than with FormBuilder, so +1.Comment #92
effulgentsia CreditAttribution: effulgentsia at Acquia commentedActually, not really sure if that's a good thing. Maybe we should just do ->setRequestMethod(), and if in the future $form_state needs more info, we provide a setter for it and have the appropriate caller use it?
Comment #93
catchI like #92.
Comment #94
yched CreditAttribution: yched commentedThanks for the clarifications in #91, my view of the form_state persistence flow was a bit naive.
#92 does indeed sound nice though.
Comment #95
dawehnerI'm fine with #92
In case nobody beats me I'll work on it in a while, but I'm currently into writing stupid tests for tokens.
Comment #96
dawehnerI did not wanted to kill the test on purpose, for now just a reupload of #85
Comment #97
effulgentsia CreditAttribution: effulgentsia at Acquia commentedImplements #92. There's a lot of related things to document, so I made some choices on what to write where. Open to feedback for how to improve them.
Comment #100
tim.plunkettI think we only snake_case'd the legacy properties, new ones like $anyErrors are lowerCamelCase
I guess we either need a getRequestMethod(), or more likely, just use $this->request_method directly.
Comment #101
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFixed #100 and started slogging through some of the other test failures.
Comment #103
dawehnerI think it should be possible to still opt into caching, with an extra parameter to bypass the validation.
Comment #105
dawehnerFix the remaining one.
Comment #106
larowlanA few general questions etc, haven't read previous reviews/comments so apologies if going over old ground
This comment doesn't read well. Suggest 'HTTP methods which are not supported' perhaps?
suggest
By default GET request cannot be cached. Set this to TRUE to allow caching of forms using a GET request.
This is not enforced on FormState::setMethod - should it be?
Should we also add a @see to setMethod()
Should we be adding a test for this too?
nit: realise its existing but 'to change to it' doesn't really make much sense?
We have to assertNoText here but no assertText (i.e. no positive assert), should we add one to ensure we're getting what we were expecting and not e.g. an error page etc.
Comment #107
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThat kind of violates the whole point of this issue. Instead, I think we need to fix the tests similarly to the interdiff in #101: to only call $form_state->setCached() when there's a logical reason to. I can work on that tomorrow if no one beats me to it.
Comment #108
dawehnerMh, that is fair. I was a little bit influenced yesterday night. I'll work on it now.
Comment #109
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedDaniel has been distracted by another issue, taking over for now :)
Comment #110
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedPosting what I have so far, since I'll switch to another issue for a bit.
I started with the patch from #101, so the interdiff is relative to that one.
Comment #113
dawehnerI added GET and HEAD, so I think this explains more what is going on.
Mh, in an ideal world yes, but also it practically never matters.
Meh, its something just dealt for developers, which run simpletests, and not had a problem on the actual site.
Not even sure how to test that properly with a limited amount of insanity.
I removed tests which IMHO simply doesn't make sense anymore and fixed some other ones,
but I'm not entirely sure ATM. what the expected result of \Drupal\system\Tests\Form\StorageTest::testImmutableFormLegacyProtection and \Drupal\system\Tests\Form\StorageTest::testImmutableForm is.
Comment #114
dawehner@effulgentsia I would be happy about the remaining ones, but I'm not exactly sure what the expected behaviour is, so just changing the test is not right.
Comment #116
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI want to scrutinize the last 2 interdiffs a bit more, but in the meantime, here's what I think we should do about those last failures.
Comment #117
dawehnerOOH that seems evil. It rather let me ask myself, whether that method should do validation. I guess that is a bit of too much babysitting.
Comment #118
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFrom http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html:
So there's nothing wrong with allowing a FAKE method. And, it's fine to cache forms for that method, since HTTP 1.1 doesn't define FAKE as a method that must be safe. What's dangerous is that $form_state->setRequestMethod() can be called with something that isn't $request->getMethod(), which means it can be tricked into doing something inappropriate for the actual request, but I don't know if or how to babysit that, because even $form_state->setRequest() or $form_state->(set|update)FromRequest() would have that problem, since you can pass in a fake $request to that. Even \Drupal::request() can be circumvented by temporarily pushing to the request stack and then popping.
Comment #119
dawehnerAnd all that is totally fine, we are not nazis. If there are advanced usecases, allow people to deal with it, but well then they need to live with the consequences.
Strictly speaking some of the new test coverage is now for code which is not touched, but honestly, new test coverage is not bad, especially
when we want to change something in the follow up issue.
We already have a change record.
Comment #120
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC + 1
--
Proposed commit message:
Core committers can obviously decide differently, but I read every comment and all those made useful contributions to the patch / approach.
Comment #123
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedConfigFormOverrideTest had two failures.
Comment #124
alexpottShouldn't this change be in the CR. Talked about this with @dawehner - this change was earlier and this patch is just making it explicit and fixing the docs.
Given point 1 - this code in
core/modules/views_ui/src/ViewUI.php
looks wrong... I think it should just be'url' => $form_url
- but according to @dawehner it's working atm. This should be a followup - or perhaps part of #2527740: Consider to remove support for #ajax['url']The docs for
path
look incorrect. I don;t think that islikely different than the $path parameter used for retrieving the initial form
Does the "callback for the generic Ajax processing path" really exist anymore?
It seems weird to me that this change has not caused a single change to any calls to drupalPostForm... Ah I see we've changed drupalPostAjaxForm and we're getting the path from
$this->drupalSettings
for calls like$this->drupalPostAjaxForm(NULL, $edit, 'editor_configure');
. Nice.Comment #125
dawehnerComment #126
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedGiven the discussion in IRC, this is back to RTBC.
Comment #127
alexpottThanks for addressing my feedback. Committed 7eb616f and pushed to 8.0.x. Thanks!
Comment #129
Wim LeersNow that this has landed, let's continue in #2524408: Remove $form_state immutability!