[Routing] Fix support for stringable parameters #42057
Conversation
src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php
Outdated
Show resolved
Hide resolved
|
It'd be nice if you could investigate and complete this PR :) |
|
Sure so we want to support everything what is stringable and not only UID explicitly, right? You prefer it as a bugfix against 5.3 ? |
|
If it's a bug fix, that could even be for 4.4. Let's see the patch on 5.4 first to decide :) |
|
This works @nicolas-grekas, but I guess, this would not work if it contains sth. like: [
'key' => value,
'array_key' => [
'key2' => new StringableObject()
]
]🤔 |
src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php
Outdated
Show resolved
Hide resolved
|
I would apply it as a bugfix because of the silent behavior. And would start with 4.4 and then patch all upper components |
|
Works for me |
|
Will start tomorrow with 4.4 and try to find all spots where we use http_build_query |
|
BTW starting from PHP 8.1 this could be fixed in PHP itself taking Serializable into account, WDYT? |
|
We should open a bug report to discuss this yes (or maybe there is already one btw?) |
|
I will check that tomorrow too 👍🏻 |
done |
668ce49 to
57d6196
Compare
| $v = (array) $v; | ||
| array_walk_recursive($v, $caster); | ||
| } elseif (\is_object($v)) { | ||
| $v = (string) $v; |
There was a problem hiding this comment.
What about array-like objects here? I'd guess that Iterators/ArrayAccess implementations would want to behave similar to (array) $v in the above case. What should happen in the case of an Iterator or ArrayAccess (for example) where the object can be converted to a string representation?
Would it be an idea to give users control over how the parameters should be rendered, perhaps through an interface or custom parameter object that can deal with this?
There was a problem hiding this comment.
For those, we should mimik what http_build_query does, nothing more. A test case would help know.
| $v = (array) $v; | ||
| array_walk_recursive($v, $caster); | ||
| } elseif (\is_object($v)) { | ||
| $v = (string) $v; |
There was a problem hiding this comment.
if the object does not implement __toString this would throw an exception which is a bc break.
and even if it did, there would be a bc break because http_build_query uses the public properties normally.
see https://3v4l.org/9cDG3
There was a problem hiding this comment.
if the object does not implement __toString this would throw an exception which is a bc break.
I added a test case and check for __toString() in 31d6937
and even if it did, there would be a bc break because http_build_query uses the public properties normally.
see https://3v4l.org/9cDG3
We could check if some public properties exists and do nothing instead of using toString()
|
Ok, what about adding it as new feature against |
I think it should be: raising deprecation in 5.4, exception in 6.0. |
|
I think we must target 5.4 and add deprecations when the parameters contain things we do not want to support anymore. |
|
I added some check and test cases for public properties: cdfd7a0 |
|
We could also check by using against 5.4 and throw a deprecation warning, and an exception in 6.0 to avoid all kind of magic we are doing here |
| @@ -298,7 +298,7 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa | |||
| if ($v instanceof \stdClass) { | |||
| $v = (array) $v; | |||
There was a problem hiding this comment.
this is breaking for classes that extend stdClass, https://3v4l.org/6e5Vj
|
Not sure how to proceed 😕 |
|
To answer the branch question, this can only be for the next version, so 5.4 as of today. |
|
Continued in #43612 |
… strings with stringables (nicolas-grekas, OskarStark) This PR was merged into the 5.4 branch. Discussion ---------- [BrowserKit][HttpClient][Routing] support building query strings with stringables | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | yes (minor) | Deprecations? | no | Tickets | Fix #26992 | License | MIT | Doc PR | - Allows using eg an instance of `Uid` as a route parameter. Replaces #42057 Commits ------- 65e2cac Add check and tests for public properties ab38bb8 [BrowserKit][HttpClient][Routing] support building query strings with stringables
In my project I added a
Ulidas query parameter and it was not added and no exception was thrown, so it was simply missing.I added a test case, and it is failing as expected:
I am also not sure about the behaviour we want, but not adding the parameter without any notice is not a good DX in my opinion. Can you please leave your thoughts how to proceed? We can also add support for
\Stringablein general? Thankscc @fancyweb @nicolas-grekas @linaori @garak
Conclusion
It looks, like
http_build_querydoes no casting!