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.
Title says it all.
Correct example
JsonResponse
+ 403 prints
{}
Where AjaxResponse fails
AjaxResponse
+ 403 prints
A fatal error occurred:
, but should print
[]
There is zero test coverage for this AFAICT.
Steps to reproduce
- Create a node.
- Go to the front page.
- Convince yourself that the
quickedit/attachments
route is working by looking at the XHR requests when you look at the front page. - Edit
quickedit.routing.yml
, change this
quickedit_attachments: pattern: '/quickedit/attachments' defaults: _controller: '\Drupal\quickedit\QuickEditController::attachments' requirements: _permission: 'access in-place editing'
to:
quickedit_attachments: pattern: '/quickedit/attachments' defaults: _controller: '\Drupal\quickedit\QuickEditController::attachments' options: _access_mode: 'ALL' requirements: _permission: 'access in-place editing' _access: 'FALSE'
- Clear all caches.
- Now again look at the XHR requests on the front page, you will see the reported problem.
Related issues
#2055937: Introduce error handling to in-place editing; if an AJAX request to commit changes fails, the user cannot recover
#77245: Provide a common API for displaying JavaScript messages
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff-2063303-32.txt | 3.38 KB | damiankloip |
#32 | 2063303-32.patch | 9.08 KB | damiankloip |
#31 | interdiff.txt | 1.66 KB | dawehner |
#31 | ajax_response-2063303-31.patch | 9.03 KB | dawehner |
#28 | ajax_response-2063303-28.patch | 8.61 KB | dawehner |
Comments
Comment #0.0
Wim LeersUpdated issue summary.
Comment #0.1
Wim LeersUpdated issue summary.
Comment #0.2
Wim LeersAdded steps to reproduce.
Comment #1
Wim LeersNote that this it not necessarily a bug in
AjaxResponse
, it could also be a bug in the routing system.I don't know enough about the routing system to pinpoint it further (without learning all of the routing system).
Comment #1.0
Wim LeersUpdated issue summary.
Comment #2
Crell CreditAttribution: Crell commented"A fatal error occurred" sounds like an actual error happened somewhere and it's being badly reported. That suggests an issue in the error handling system to me.
Comment #3
Wim Leers#2: true. To that end, #1158322: Add backtrace to all errors could be highly helpful.
Comment #4
Crell CreditAttribution: Crell commentedI know Symfony and Silex have very good debug output in case of uncaught exceptions. Symfony also has a nice debug toolbar. We should perhaps look into both of them and see if there's code we can borrow, or at least concepts.
Comment #5
Wim Leers#4: that'd be AWESOME :)
Comment #6
BerdirDrupal\Core\Controller\ExceptionController
The method it's looking for is "on403drupal_ajax", which is a rather weird method name :) Maybe link that to Json on that level? We do expect json, right?
That whole thing seems quite un-extendable? What would have have to do if I want to return XML, or actually, request XML and get a 403 or 404?
Comment #7
Wim LeersGreat analysis — thanks so much, Berdir! :)
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedSo, how is
Drupal\Core\EventSubscriber\ViewSubscriber
supposed to work? It has similar code:... but defines a
onAjax()
but noondrupal_ajax()
. I suppose we are not actually using the correct mimetype yet?I echo Berdir's sentiment: we are missing a proper rendering layer. Symfony Full Stack doesn't have a proper rendering layer either, but at least it can guess templates based on the format of the incoming request (as implemented in
Sensio\Bundle\FrameworkExtraBundle\EventListener\TemplateListener
.Also, is there any reason we are not using the proper
Request::getRequestFormat()
/Request::setRequestFormat()
instead of hardwiring our content negotiation service everywhere?Comment #9
Crell CreditAttribution: Crell commentedDamien: We have an open issue with no code yet to standardize on getRequestFormat() properly. You're correct that we should be.
Also, we're discussing in the WSCCI channel as we speak about how ViewSubscriber needs to die a painful death. :-) It was never intended to live this long; refactoring HtmlPageController and ViewSubscriber (which are quite muddled right now) is something I've been thinking about the past few days. ExceptionController is equally hackish. It's out of scope for this issue to rewrite, but I want to try rewriting them.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commented@Crell: Is there any specific reason why we are wiring all this in "wrapping" controllers, instead of using the
KernelEvents::VIEW
event like Symfony full stack does (and likeViewSubscriber
currently does)?Also, the default
Symfony\Component\HttpKernel\EventListener\ExceptionListener
handles exceptions as sub-requests to a special controller, which makes a ton of sense to me too. But of course, that requires separating rendering from the controllers, and brings us back to the previous point.Comment #11
Crell CreditAttribution: Crell commentedDamien: HtmlPageController was written with the expectation that it would get replaced by SCOTCH, but that's not happened. ViewSubscriber was written 7 months earlier with the expectation that it would get replaced as well, by SCOTCH or otherwise. Refactoring both of those so that we're using the view event properly, with a classed object, is exactly what I want to do. That's tangentially related to this issue but would likely overlap with it. (I'm waiting for the title issue to get in so that I'm not editing the exact same lines at the same time.)
Comment #11.0
Crell CreditAttribution: Crell commentedadded related issues
Comment #12
BerdirWhat's the state here? This is still a problem and makes debugging ajax requests very painfull. It's not just a problem for 404/403 but also real errors where right now, ajax actions just silently fail.
Comment #13
Crell CreditAttribution: Crell commentedI believe #2323759: Modularize kernel exception handling will allow us to address this issue.
Comment #14
dawehnerSo
Comment #15
haydeniv CreditAttribution: haydeniv commentedSo #2323759: Modularize kernel exception handling is in so active now?
Comment #16
Crell CreditAttribution: Crell commentedCan we get a failing test here to confirm it's still an issue and wasn't fixed implicitly as part of the exception refactor? If it's still an issue we need a test anyway but that should make it much easier to fix.
Comment #17
webchickComment #18
Wim LeersThere you go.
(Still a fatal error.)
Comment #19
Wim LeersUpdated the STR in the IS.
Comment #21
dawehnerLet's see whether this works. On top of that I wonder whether we this will cause issues in ie9, see #2339491: Ajax requests on forms with files fail on IE 9
Comment #22
Wim LeersWoot! Can't wait to see the test results :)
Comment #23
dawehnerAt least one of the failures of the quick test ones, will be green.
Comment #25
dawehnerMeh. Not sure whether {} or [] should be the prefered way when there is no text. It seems to be that symfony decided to go with the first one, for some reason? Now this
patch uses "[]" but we could easily switch.
Comment #27
Crell CreditAttribution: Crell commentedRemember, it's a JSON object. A bare [] isn't valid JSON. A bare {} is a valid JSON null object.
Comment #28
dawehnerMaybe now.
Comment #30
Wim LeersHah, it used to be
[]
, but I guess that's because the old AJAX system was also wrong :)Only test failures remaining in a single test! :)
Comment #31
dawehnerThere we go.
Comment #32
damiankloip CreditAttribution: damiankloip commentedThis patch is looking good to me. Just one thing IMO, we should return
{"error": "Some fatal error message here"}
instead of just["Some fatal error message here"]
. This keeps things more consistent but also allows you to get this withresponse.error
and notresponse[0]
Comment #33
Crell CreditAttribution: Crell commentedAnd we even get to fix that thing, too. Nice!
Comment #34
Wim LeersHurray!
RTBC+1
Comment #35
catchCommitted/pushed to 8.0.x, thanks!