8000 [Routing][TwigExtensions] Inconsistency of path dealing with boolean · Issue #21409 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing][TwigExtensions] Inconsistency of path dealing with boolean #21409

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
remi-blaise opened this issue Jan 25, 2017 · 11 comments
Closed

[Routing][TwigExtensions] Inconsistency of path dealing with boolean #21409

remi-blaise opened this issue Jan 25, 2017 · 11 comments
Labels
Bug Routing Status: Needs Review TwigBridge

Comments

@remi-blaise
Copy link
Contributor
remi-blaise commented Jan 25, 2017 •
edited
Loading
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.2.1

Hello!

In Twig, the path function accepts true as a boolean argument and then outputs "1" in the url, which is then consistently proccessed by the Router, while when passing false it outputs "false" in the url, which is interpreted as a string and do not matches.

Is this intended, a bug or an inconsistency?
@ogizanagi
Copy link
Contributor
ogizanagi commented Jan 25, 2017 •
edited
Loading

I can't reproduce the issue :/.
Using a boolean as a route parameter will simply resolve to "1" in the url if true was provided, or "" if false ("0" in case of a query parameter).

I.E, given the following test route -> /test/{foo}/bar:

  • path('test', { foo: true, baz: true }) =>/test/1/bar?baz=1
  • path('test', { foo: true, baz: false }) =>/test/1/bar?baz=0
  • path('test', { foo: false, baz: false }) =>/test//bar?baz=0

8000
@remi-blaise
Copy link
Contributor Author

First, thank you for your answer.

Well, for me:

  • {{ path('api.approve_game', {'id': game.id, 'approved': 0}) }} => /api/approve_game/53/0
  • {{ path('api.approve_game', {'id': game.id, 'approved': true}) }} and {{ path('api.approve_game', {'id': game.id, 'approved': 1}) }} =>/api/approve_game/53/1

But {{ path('api.approve_game', {'id': game.id, 'approved': false}) }} now throw this error:

An exception has been thrown during the rendering of a template ("Parameter "approved" for route "api.approve_game" must match "[^/]++" ("" given) to generate a corresponding URL.")

My route:

    /**
     * @Route("/approve_game/{id}/{approved}", name="api.approve_game")
     */
    public function approveGameAction(Request $request, Game $game, bool $approved)

So what's the hell?

@ogizanagi
Copy link
Contributor

{{ path('api.approve_game', {'id': game.id, 'approved': false}) }}
is throwing because the default requirement for parameters is indeed [^/]++ (see route requirements. Here is also mentionned the default value).
But as the exception message states, the given string was an empty one (""), which is the expected behavior described in my first comment :)

@robfrawley
Copy link
Contributor
robfrawley commented Jan 25, 2017 •
edited
Loading

@ogizanagi Is it truly expected behavior to treat conversion of true and false differently?

  • true => 1
  • false => ""

That seems inconsistent to me, regardless of original implementation. Is there a good reason for this or would it be possible to make the behavior consistent in a future major version release, when BC can be broken for the sake of a consistent approach.

@javiereguiluz
Copy link
Member
javiereguiluz commented Jan 26, 2017 •
edited
Loading

I agree. This looks inconsistent. Bad news is that fixing this will be a BC break. Should we:

  • Keep doing true -> "1" ... but also do false -> 0 ?
  • Keep doing false -> "" ... but also do true -> "" ?

@stof
Copy link
Member
stof commented Jan 26, 2017

Well, this is just the way casting a boolean to string works in PHP. The rule is not true => 1 but true => "1"

@stof
Copy link
Member
stof commented Jan 26, 2017

And your issue is related to the fact that route placeholders are meant to be strings. Passing booleans there is supported only because casting is supported, but then it follows the PHP casting rules. If you don't want these casting rules, pass a string directly.

@robfrawley
Copy link
Contributor

I understand why it behaves this way, I simply disagree that is should remain working this way, moving forward.

@fabpot
Copy link
Member
fabpot commented Jan 26, 2017

As @stof said, those values are strings, nothing else. Passing a boolean there does not make sense in the first place.

@javiereguiluz
Copy link
Member

Given the comments above, I'm closing this as "won't fix" then. First, because this is in fact PHP's behavior, not Symfony's ... and second, because the solution is "simple": not passing boolean values as route placeholders because they are not supported (only strings and numeric values).

@remi-blaise remi-blaise mentioned this issue Jan 27, 2017
Closed
@remi-blaise
Copy link
Contributor Author

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Routing Status: Needs Review TwigBridge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @javiereguiluz @stof @ogizanagi @robfrawley @remi-blaise @carsonbot
0