Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#2263569: Bypass form caching by default for forms using #ajax. is moving #ajax forms away from relying on cached forms. File uploads currently always require the form to be cached.
Proposed resolution
Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache.
Remaining tasks
Given that the file upload limit might exceed the$_POST
limit or be near it, the actual rest of the form submitted data might be skipped so we need some protected against non valid submissions:Figure out the amount of files, so we can show some information that a new file got uploaded. This could be solved by ManagedFile tracking the amount of files before and after
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#69 | 2500527-followup-69.patch | 886 bytes | tim.plunkett |
#63 | interdiff.txt | 16.3 KB | dawehner |
#63 | 2500527-63.patch | 28.6 KB | dawehner |
#57 | interdiff.txt | 615 bytes | tim.plunkett |
#57 | 2500527-56.patch | 28.69 KB | tim.plunkett |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCritical per #2465053-65: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state, unless we decide on a different solution for that issue.
Comment #2
yched CreditAttribution: yched commentedAs a side note, FileWidgetAjaxController is actually fairly misnamed, since it's not related to file field widgets, but rather to the lower-level ManagedFile form element type.
Comment #3
dawehnerTalked with @effulgentsia about that issue
Currently
\Drupal\file\Controller\FileWidgetAjaxController::upload
has a couple of additional properties on top of submitting the form:$_POST
limit or be near it, the actual rest of the form submitted data might be skipped so we need some protected against non valid submissions:Applied for a grant to try to get this issue running
Comment #4
yched CreditAttribution: yched commentedI'm very possibly wrong, or simply irrelevant, but : last time I looked, I had the impression that ManagedFile was about single file uploads, while FileWidget added the extra logic for handling multiple-valued fields ?
Comment #5
tim.plunkettComment #6
dawehnerEhm yes, I was just trying to make the point what its doing on top of processing the form
Comment #7
dawehnerif (empty($request_form_build_id) || $form_build_id !== $request_form_build_id) {
I think if we want to change the way how this works, its not critical, compared to the rest of the issue
Note: This patch has #2263569: Bypass form caching by default for forms using #ajax. applied.
Comment #10
dawehnerThe failure in UserPasswordTest is easy, we should respect current query parameters when we construct the #ajax URL options, so that user_pass_reset can be carried over.
The second fail is more tricky.
So we have a couple of upload/remove buttons and so ajax settings attached to it.
The test first adds a couple of new files and then tries to remove them.
During that time ideally all the ajax settings for all the buttons should always point to the newly generated build IDs.
With that patch they though don't because we just render a particular subset, and so we just get the updates for the fields which are re-renderer. In the debugger its looks like that
(blue are the changed ones)
Comment #12
dawehnerJust rerolling
Comment #13
larowlanLove the test coverage this patch adds - swoon.
A few comments:
Ridculous? :-)
The language here is a bit disjointed
Do ajax callbacks support service id notation like we do in other places? Would mean we could use DI instead of a static method
Comment #16
dawehnerI'll leave this out :) Its not a problem of this patch ...
Tried to improve it
There is the
::foo
synatx but it relates to the form class itself, which is not the case for generic form elements.Comment #18
dawehnerFor now this is just a reroll with a lot of debugging code in there.
Comment #19
dawehnerHere is a report of my current state of debugging, days of fun:
Steps to reproduce the test failure with the patch:
and you end up with this odd behaviour. Why this happens is not clear yet.
PS: Other problems which exists and might become more problematic in the future:
edit-field-field1--4-ajax-wrapper
. This is also the case in HEAD so its just "random" that the browser selects the first one#prefix, #suffix
wrapperComment #20
dawehnerThis was the actual problem ... @effulgentsia suggested to remove the checking for the build ID, and this fixed the problem.
Comment #21
dawehnerFirst follow up:
Comment #22
dawehnerOne bit which is patch is doing is to remove some checking for truncated POST requests ... this adds it bad, but I'm not sure how to test it already,
because it basically requires a specific PHP configuration, with post_max_size === max_upload_size
Not sure whether a pure unit test with a faked up request object is a good protection ...
Comment #23
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think we should remove #22 from this issue's scope and do it in #2507019: Determine how much POST truncation protection is needed and add tests for it. I think for this issue, we can allow the regression of not getting that friendly message, and then restore that functionality properly in #2507019: Determine how much POST truncation protection is needed and add tests for it.
Comment #25
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAfter some more digging, I changed my mind about that. With JS enabled, in HEAD, if you try uploading a file > post_max_size, you get a friendly validation error. With both #20 and #22, you get a silent failure. In the case of #20, because what ends up getting returned is HTML rather than JSON (because due to the empty $_POST, there's no form for which $form_state->isProcessingInput() returns true, so the code never flows to AJAX processing). In the case of #22, because nothing handles the BadRequestHttpException, so an empty
{}
is returned.Here's a patch (interdiff relative to #20) that works, but it could use some cleanup. Testing this should just be a matter of enhancing FileManagedFileElementTest with a drupalPostAjaxForm() submission of a file larger than post_max_size.
Comment #26
effulgentsia CreditAttribution: effulgentsia at Acquia commentedA subtle difference here is that the code being removed from HEAD only runs when you use a file widget, whereas the code being added by patch is run whenever you have an $ajax_form_request with an empty $_POST. So the error message is an assumption. But not an entirely unreasonable one: in the real world, when do you ever exceed post_max_size without a file upload? Not sure how much we want to worry about the edge case of that not being true here vs. in #2507019: Determine how much POST truncation protection is needed and add tests for it.
Comment #27
dawehnerIt feels a little bit bad to use translatable exceptions but yeah I guess this this user facing text.
Comment #29
alexpott@catch, @effulgentsia, @webchick, @xjm and I discussed this issue and decided to keep it critical since it is part the work to stop setting form cache on get requests. See #2502785: Remove support for $form_state->setCached() for GET requests
Comment #30
dawehnerehem sorry this was a cross-upload of a patch ... so better ignore #28
Comment #31
dawehnerWe should at least ensure that we don't take over all
BadHttpRequestExceptions
Here is some first start of a test coverage, ... the question is, what should we do with the non JS upload case ...
Comment #33
dawehnerLet's fix the test coverage ... This is almost never a bad idea.
Comment #35
dawehnerLet's use a better API function and at the same time, get some more debugging output in here ...
Comment #36
yched CreditAttribution: yched commentedPatch looks awesome
I might very well be wrong (away from my coding env atm, sorry for the noise if that's the case), but isn't this adding "multiple value" logic in ManagedFile, which otherwise only caters for single uploads ? (i.e we end up with logic in the generic, lower-level ManagedFile that's coupled to the form structure of the higher-level and more specific FileWidget ?)
Comment #38
dawehnerMh you are right about that, but it doesn't make the situation any worse than before, if I understand it correctly.
The previous controller also contained such a logic, and at the same time, was also intended to be used for a single file. Do you have an obvious way to fix that?
Comment #39
dawehnerAlright, so the upload_max_size is 4x less than the post_max_size ..
Let's try something like this: Adapt the ini settings to be the same, so we can trigger the post case.
Comment #41
dawehnerMh, this test seems not to work on the testbot :(
Comment #42
dawehnerYeah no idea, also setting up those values 8MB 2MB locally doesn't help to reproduce the problem
Comment #43
effulgentsia CreditAttribution: effulgentsia at Acquia commented#39 needs a reroll. But I applied it successfully on yesterday's HEAD, and ran the failing test locally, and it passes on my machine as well. Both on PHP 5.5 an 5.4. So, yes, something interesting happening on bot. Probably makes sense to open a tester issue and start throwing some debugging code at bot to see why it's behaving differently than local.
Comment #44
effulgentsia CreditAttribution: effulgentsia at Acquia commentedJust a reroll.
Comment #45
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIt's a pre-existing condition of HEAD that we don't have test coverage for uploading a file that exceeds post_max_size. Given that the added test is working locally but failing on bot, I don't think we should hold up a critical on figuring out what's different about the bot. So, removing it from here, but let's open a separate issue to add it. That separate issue can proceed independently of this one (i.e., land either before or after).
Comment #47
tim.plunkettWas going to leave a nitpicky review, but fixed them instead.
Considering #45, that's all there is, right?
Comment #48
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC + 1, some nit picks and questions, but overall looks really great!
nit nit nit - I thought we usually omitted 'you' or used 'we' instead.
not a bug - This is more question for myself:
So even if I use the StringTranslation trait I still inject the string_translation service?
/me wonders ...
Nice :)
Comment #49
tim.plunkett48.2, yes that is best practice, and especially for a service. I don't bother for controllers or plugins.
Comment #50
dawehnerFair, its still a bit sad that this is not something obvious to check for.
Added a follow up for that #2510436: Test uploading a file > ini_get(post_max_size)
My personal rule of thumb is: For services inject the dependencies, for other code like controllers / plugins don't.
OH yeah that is exactly what tim wrote.
Comment #51
Wim LeersNit: s/renderering/renderer/
:O
I totally did not know that
X-Status-Code
was a thing.Still, why not use
setStatusCode()
directly?Nit: s/post/POST/
Nit: 80 cols.
Nit: s/amount/number/
Comment #52
Fabianx CreditAttribution: Fabianx for Drupal Association commented#51.2: Me, neither, but its only allowed for Exceptions handled by HttpKernel:
core/vendor/symfony/http-kernel/HttpKernel.php: if ($response->headers->has('X-Status-Code')) {
core/vendor/symfony/http-kernel/HttpKernel.php: $response->setStatusCode($response->headers->get('X-Status-Code'));
core/vendor/symfony/http-kernel/HttpKernel.php: $response->headers->remove('X-Status-Code');
Comment #53
dawehnerThank you tim!!
Sadly I can't point to the code nor handbook on symfony.com, but symfony converts the exception to the corresponding http status code, so 500 for this example by default.
You need to use that header in order to bypass that and do exactly what you want.
Comment #54
Fabianx CreditAttribution: Fabianx for Drupal Association commented#53: http://symfony.com/doc/current/reference/events.html#kernel-exception explains it. Thanks for the hint on the handbook :). (With "X-Status-Code site:symfony.com" I was able to find this easily.)
Comment #55
alexpottTo
t()
or not tot()
? We seem to be split as to whether we translate or not. I'm not sure what is best here.Not used
Comment #56
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRe #55.1: We t() that string in HEAD, so I don't think this issue should be the one to change that. But, do we need a followup for not t()'ing it inside an exception?
Comment #57
tim.plunkettWe usually don't translate exception messages, except this exception is always caught and then we call
$this->drupalSetMessage($exception->getMessage(), 'error');
on it.Removed stray import.
Comment #58
joelpittetI think we do
t()
, by no means am I a translation expert but that seems like it's a user interface error message that an editor in a different language would likely see.Comment #59
dawehnerYeah its an error which potentially could happen by a normal action of an user, so ...
Comment #60
Wim Leers+1 to #58 & #59.
Comment #61
effulgentsia CreditAttribution: effulgentsia at Acquia commented+1 that this string needs to be t()'d. What I'm less clear on is whether it's okay to be t()'ing it at the point of throwing the exception or if we need a followup to move the t() to the place that catches the exception, except I also don't know if we have or want to introduce the pattern of adding placeholder arguments into an exception for deferred invocation of t(). Assigning to @alexpott for feedback about that.
Comment #62
alexpott@effulgentsia yep I'm concerned about the use of
t()
in the exception. It feels very wrong to have translated exception messages but this is hardly the only place in core. But two wrongs don't make a right. https://www.drupal.org/node/608166 says we should never translate exception messages.I think we should have an exception that extends BadRequestHttpException and has a method for storing the size. And then the translated message should be prepared where this exception is consumed.
Comment #63
dawehnerGood idea alex! More semantic exceptions!
While doing that I think it would be a great to not store the formatted output.
Comment #64
Fabianx CreditAttribution: Fabianx for Drupal Association commentedComment #65
Wim LeersMuch better. Thanks Alexes :)
Comment #66
catchLooks great and like the exception changes in #63.
Committed/pushed to 8.0.x, thanks!
Comment #68
dawehnerYeah!!!
Comment #69
tim.plunkettOh nice! I like that a lot better.
We can remove these now though.
Comment #70
tim.plunkettFollow-ups are better. #2511354: Remove StringTranslationTrait from FormBuilder
Comment #71
nod_Comment #73
Wim LeersThis caused a regression: #2540850: (regression) EditorImageDialog alignment & captioning are not working.