Background Information
Some important background information to understand the problem:
- Drupal now sets a content-length header for most responses (https://www.drupal.org/node/3298551). The length is set in web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php. The length is
strlen($content) - NGINX strictly follows the content-length header. If content-length=5 and it receives 6 characters, it truncates the last one.
- It is possible to send data outside of the AJAX response. For example, whitespace before the open PHP tag is sent.
Problem/Motivation
We noticed that some AJAX calls were failing on a Drupal site. The browser was getting a response. But, the last two characters of the response were truncated. This caused an invalid response so the AJAX call failed.
We noticed that there were two characters of whitespace added to the beginning of AJAX responses. For example, Drupal built a response like:
[{"command":"update_build_id","old":"**********","settings":null}]
But the browser received a response like:
[{"command":"update_build_id","old":"**********","settings":null
Note the two whitespace at the beginning and the missing characters at the end. We back-tracked this to whitespace introduced before the open PHP tags on a custom .theme file.
The obvious solution is "don't have whitespace before the PHP tag". Given the number of issues, that happens more than it should. Drupal adds the content-length header. Drupal can do more to make sure Drupal is sending the correct length.
Steps to reproduce
- Set up a Drupal site. We are using Lando's Pantheon recipe with NGINX.
- Use a default admin theme. Let's say Claro.
- Edit a view in the Views admin UI. Click any link that makes an AJAX call. For example, change the title. Confirm that it works.
- Edit web/core/themes/claro/claro.theme. Add whitespace before the open PHP tag (" <?php")
- Retest the link in the Views admin UI. It should fail. You won't see any change on screen.
- Use the browser developer tools, network tab. The request will likely have status 200. But, if you dig into the response, you will see a syntax error. If you read the response, it will be missing characters at the end.
Proposed resolution
Protect AJAX responses so only the content in the response is sent. For example, I added AjaxResponse->send() to override Response->send(). In AjaxResponse->send(), I:
- ob_clean() to dump anything in the output buffer (like whitespace that happened to be before the open PHP tag)
- execute the parent send() method
For reference:
- web/core/lib/Drupal/Core/Ajax/AjaxResponse.php extends vendor/symfony/http-foundation/JsonResponse.php
- JsonResponse extends vendor/symfony/http-foundation/Response.php
Remaining tasks
I don't know the implications of doing a ob_clean() before sending the response. Could someone with more expertise please weigh in on any unintended consequences?
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | Screenshot from 2024-12-19 06-08-15.png | 820.96 KB | jasonsafro |
Issue fork drupal-3494148
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3494148-ajax-response-not
changes, plain diff MR !10606
- 11.x
changes, plain diff MR !10605
Comments
Comment #2
jasonsafro commentedComment #3
jasonsafro commentedAdding the following to web/core/lib/Drupal/Core/Ajax/AjaxResponse.php
Comment #4
jasonsafro commentedComment #5
jasonsafro commentedComment #6
jasonsafro commentedComment #7
jasonsafro commentedComment #8
quietone commentedChanges are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies. So, changing this to 11.x.
Comment #9
jasonsafro commented@quietone Thanks!
Comment #10
catchBumping this to major, while it's technically a workaround for extra whitespace in PHP code, it would make everyone's lives a lot easier if it works well.
Comment #14
jasonsafro commentedComment #15
dimitriskr commentedSetting priority back to Major per #10
Comment #16
longwaveThis is neat, but it does hide the real problem. I thought about a different solution when I ran into this before - leading whitespace in a .module file - but never implemented it: when we
include.module or .theme files, we could wrap them inob_start()andob_end_clean(), and optionally throw a warning to tell developers that there is a problem with their file?Comment #17
longwave#3409885: Uncaught ajax.js error / exception is possibly fixed by this as well.
Comment #18
jasonsafro commentedScreenshot with notes to help in testing / debugging.
Comment #19
jasonsafro commented@longwave You are not wrong. I'm not even sure what is "the real error". Is the real error that someone left some whitespace outside the PHP tags? Or, is the real error that Drupal can miscount the content-length?
If there's a better solution, I'm all for it. But, I'd like to make some change. Debugging this error was not fun or easy.
Comment #20
catch#16 is worth a try and it would be easier to add a test. Since it could potentially run on every request, we should probably do it in an assert rather than actual logging?
Probably need to support include files too but that can be done in ModuleHandler::loadInclude().
Comment #21
smustgrave commentedComment #22
jasonsafro commented@longwave @catch - Are either of you working on the solution that you discussed?
Comment #23
jasonsafro commented@longwave proposed that we add some checks to tell programmers if there is extra whitespace in their files. This is an interesting idea. Personally, I think that test belongs in a linter. I'm open to discuss but have not been able to get ahold of @longwave.
I'm going to move forward with the solution that is ready to use. It resolves a difficult to debug issue that is tied to half a dozen tickets. I'd appreciate any feedback.
Comment #24
smustgrave commentedStill would need test coverage though.
Thanks.
Comment #25
catchSo I agree that we should add linting for this, it could probably be added to phpcs (or maybe there's an existing rule). It is worth an issue against https://www.drupal.org/project/issues/coder and then we'd need to make sure that runs on the gitlab templates.
Having said that we added the content length header not that long ago, and a surprising number of people have run into this issue, and it's not easy to diagnose, so I think there could be value to the workaround here even if we also open a follow-up to remove it again in Drupal 12 or 13.
On test coverage, I originally though about testing the actual AJAX system, but I think the problem is webserver-specific (e.g. some are permissive with mis-matched content lengths, some or not), and ideally we want the test to fail on all environments.
Something like:
- create a test module (add it to core/modules/system/tests/modules) - it has a .module file with whitespace before the opening <?php tag, doesn't need any more than that apart from .info.yml
- add an extra method to core/tests/Drupal/FunctionalTests/HttpKernel/ContentLengthTest.php
- in that method, enable ajax_test module and the test module added above, request an AJAX url, and check that the content length matches the content length header (or a fixed module that is equal to the content length header).
Or maybe a bit cleaner, would be to add an AJAX controller to http_middleware_test module and hit that URL instead, then the content length can be controlled - e.g. it won't break if someone changes ajax_test module for some other reason then.
Comment #26
jasonsafro commented@catch I know you primarily responded about testing. But I also want to get the best fix in place. And you are right. My "fix" doesn't really fix the problem. My fix makes the problem go away by dumping anything already in the output buffer. Maybe we can do better.
Would you prefer a patch that checks the size of the output buffer and then does *something*? For example, check line 48 of web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php. I could replace:
with
I could also throw an exception if the buffer isn't empty or do something else if you have a good idea.
Comment #27
catchI don't think we should throw an exception because it would cause sites that currently work (because their webservers are permissive), to not work. But we could add an assert() so it fails hard on local development environments and tests, and also log a warning (for when assertions are off).
This seems... more honest?