Either allow string controller results or provide better error handling

A KernelEvents::VIEW subscriber that maps the (escaped) string to a simple ['#markup' => $string] array makes sense to me. This is consistent, leverages Symfony, and doesn't require the Render API to have to deal with strings.

So, something like this.

Oh, #2555931: Add #plain_text to escape text in render arrays already landed. That allows me to simplify this :)

Also rebased. Patch should actually apply now.

Mh, for a while I thought a string would result in a response, but well this patch indeed converts to a render array. Mh IMHO this is too much magic without that much of
a pratical win. I like the explicit over implicit rule of PIP 20 (https://www.python.org/dev/peps/pep-0020/).

How easy would people just create a string together, then its converted to a render array and then cached ... and we have NO cacheability metadata at all,
and boom another non obvious security issue.

+++ b/core/core.services.yml
@@ -940,10 +940,6 @@ services:
-  accept_negotiation_406:
-    class: Drupal\Core\EventSubscriber\AcceptNegotiation406
-    tags:
-      - { name: event_subscriber }

@@ -965,6 +961,17 @@ services:
+
+  accept_negotiation_406:
+    class: Drupal\Core\EventSubscriber\AcceptNegotiation406
+    tags:
+      - { name: event_subscriber }

IMHO an out of scope change :)

#5: True. But in that case, I'd argue the answer is what I said on Twitter: for demo purposes, you should do new Response('Hello World'). I think we can leave it up to catch to decide whether this is acceptable overhead or not. If we don't do it here, we'd need to do it in Renderer::doRender(), but this surely has much more overhead. But we're still only talking thirteen additional function calls (I quickly profiled it). I'm happy to be trout-slapped for making Drupal 8 13 function calls slower if that one day yields a noticeable difference ;) :P

#6: You're right on both counts. First count: this was a very quick PoC. Second count: some commit totally screwed up the nice ordering/consistency in those services :( It annoyed me so much that I couldn't help fixing it. Turns out it was #2472323: Move modal / dialog to query parameters. Which I helped in. Wim--

I think the usecase of doing new Response('Hello World'). is a really good one, code is straightforward and the patch still applies, setting to RTBC.

PIFR Fail, back to RTBC.

Every listener fires on every request - why should we slow down the request for something we are not using?

Also there are no tests.

Every listener fires on every request - why should we slow down the request for something we are not using?

Yep, this is the main concern here.

We could use it in some of the test responses. But generally this would not be recommended to be used precisely because it'd lack cacheability metadata and therefore these responses would need to be uncacheable.

However, repeating what I wrote in #7:

I think we can leave it up to catch to decide whether this is acceptable overhead or not. If we don't do it here, we'd need to do it in Renderer::doRender(), but this surely has much more overhead. But we're still only talking thirteen additional function calls (I quickly profiled it).

Thoughts?

It doesn't belong in EarlyRenderingControllerWrapperSubscriber.

Isn't the real problem that Symfony's error message sucks? (When no VIEW subscriber for a controller result that is not a Response.)

So what about doing something like this?

This has a test, so removing the tag.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ControllerStringExceptionSubscriber.php
   @@ -0,0 +1,38 @@
   +   * @throws \InvalidArgumentException
   +   *   Throwns when the
   

   I think this needs more words?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ControllerStringExceptionSubscriber.php
   @@ -0,0 +1,38 @@
   +    // Choose a quite low priority so all other view subscribers run first.
   +    $events[KernelEvents::VIEW] = ['onView', -256];
   

   This eventsubscriber runs for every request, right? So why would we want this to run with low prio? Shouldn't we do this with high prio to reduce stack traces when this fails and make debugging easier?

   If that's not the case, can we document why we want the low priority?

Thank you for your review!

If that's not the case, can we document why we want the low priority?

Let me quote the code:

+ // Choose a quite low priority so all other view subscribers run first.
Doesn't this make sense?

Let me quote the code:

+ // Choose a quite low priority so all other view subscribers run first.
Doesn't this make sense?

Well, I don't understand why the others are running first. Can the other view subscribers transform a string into a correct response? I guess they could and we should just say that's why instead?

Well, I don't understand why the others are running first. Can the other view subscribers transform a string into a correct response

Well right, this is the purpose of this event. Subscribers can convert the controller result into a proper response.

Ok, thanks for holding my hand there :). Do you think it would make sense to expand the comment to:

Other view subscribers can transform the controller result from a string into something that the rendering system understands. Use a low priority so that those other view subscribers can run first.

I don't think we should mention 'all' view subscribers, since we can't be sure what weight contrib/custom modules use for their view subscribers.

Thank you for reporting this problem. We rely on issue reports like this one to resolve bugs and improve Drupal core.

Since there has been no activity here for over 8 years we are asking if this problem persists on a currently supported version of Drupal. To help, add a comment explaining if the problem still occurs or not. Any extra detail you can provide can help others who experienced this.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!

I think this still makes sense to do. To me it seems like the next steps are rerolling and adding a merge request + improving docs. Tagging as novice to do that.

Testing this on HEAD and the error message is much more helpful in 2025:

Symfony\Component\HttpKernel\Exception\ControllerDoesNotReturnResponseException: The controller must return a "Symfony\Component\HttpFoundation\Response" object but it returned a string ("string"). in () (line 98 of core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php). 

This comes from symfony's HttpKernel.

IMO this is even more helpful than the error message in the patch ("The result of a controller cannot be a string.") because it gives instructions on what it should be.

With that in mind, maybe we can just close this out now?

I agree with @acbramley, I think this can be closed as outdated because that new error seems like it makes a lot of sense.