[HttpKernel] Make the behaviour of surrogate renderers consistent in all environments #16283
Conversation
| @@ -64,6 +64,10 @@ public function __construct(SurrogateInterface $surrogate = null, FragmentRender | ||
| public function render($uri, Request $request, array $options = array()) | ||
| { | ||
| if (!$this->surrogate || !$this->surrogate->hasSurrogateCapability($request)) { | ||
| if ($uri instanceof ControllerReference) { | ||
| $this->checkNonScalar($uri->attributes); |
There was a problem hiding this comment.
Instead of making checkNonScalar() protected in order to call it here, we could generate the uri instead:
if ($uri instanceof ControllerReference) {
$uri = $this->generateFragmentUri($uri, $request, false, true);
}
return $this->inlineStrategy->render($uri, $request, $options);This way, however, _locale and _format might not be passed properly as the following code in the InlineFragmentRenderer wouldn't be called:
foreach (array('_format', '_locale') as $key) {
if (isset($attributes[$key])) {
$reference->attributes[$key] = $attributes[$key];
}
}I'm not sure if this is an issue. Perhaps it'd actually be good, if we're trying to make the dev/non-dev behaviour consistent for surrogate renderers.
There was a problem hiding this comment.
Any thoughts on this?
There was a problem hiding this comment.
I prefer your current solution. This looks good to me.
… all environments Surrogate renderers cannot work with objects. As renderers should be interchangable, we should not allow passing objects to the fallback renderer either.
8522159 to
0b489aa
Compare
|
Isn't this a BC break ? |
|
@stof if anyone relied on this behaviour he'd run into issues after deploying his app to production. Passing objects as uri attributes might have worked on dev with the inline renderer, but it would fail in prod with a real reverse proxy present. |
|
ping @symfony/deciders |
|
👍 |
|
I would still consider this as a BC break. What people only using the inline renderer? |
|
@jakzal Can you work on submitting a PR on master for this change and generate the proper deprecation notices? |
|
@fabpot But who would run their application only in the dev environment (as the application would break in prod they would probably never deploy it, would they?)? |
|
@fabpot this change only affects surrogate renderers. The inline renderer is only used as a fallback here. |
|
Sorry. I only just got what you were saying. Indeed, people might have relied on this behaviour in prod if they chose not to use a real reverse proxy. I'll work on deprecation messages. |
|
Deprecated in #17611 |
|
Closing in favor of #17611 |
Surrogate renderers cannot work with objects (rule introduced in #8437). As renderers should be interchangable, we should not allow passing objects to the fallback renderer either.