Skip to content
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.

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

Parameters without defaults are not required to generate URL in some cases #363

Open
RealJTG opened this issue Nov 14, 2019 · 2 comments
Open

Comments

@RealJTG
Copy link

RealJTG commented Nov 14, 2019

Route definition

test_route:
    path: /test/{required1}/{required2}/{optional}
    defaults:
        _controller: '\Some\Class::method'
        optional: 0
    requirements:
        optional: \d+
        required1: \d+
        required2: \d+
    methods:
        - GET
    options:
        expose: true

translated into this

fos.Router.setData({
    "base_url": "",
    "routes": {
        "test_route": {
            "tokens": [
                ["variable", "\/", "\\d+", "optional"],
                ["variable", "\/", "\\d+", "required2"],
                ["variable", "\/", "\\d+", "required1"],
                ["text", "\/test"]
            ],
            "defaults": {
                "optional": 0
            },
            "requirements": {
                "optional": "\\d+",
                "required1": "\\d+",
                "required2": "\\d+"
            },
            "hosttokens": [],
            "methods": ["GET"],
            "schemes": []
        }
    },
    "prefix": "",
    "host": "localhost",
    "port": "",
    "scheme": "http",
    "locale": "ru"
});

Cases:

Execute: Routing.generate('test_route')
Expected result: "Uncaught Error: The route "test_route" requires the parameter "required1" (or required2)
Actual result: "/test"

Execute: Routing.generate('test_route', {required1: 1})
Expected result: "Uncaught Error: The route "test_route" requires the parameter "required2"
Actual result: "/test/1"

Works as expected when setting an optional parameter:

Execute: Routing.generate('test_route', {optional: 1})
Expected result: "Uncaught Error: The route "test_route" requires the parameter "required1" (or required2)
Actual result: "Uncaught Error: The route "test_route" requires the parameter "required2"

Setting optional = false by default here https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/blob/master/Resources/public/js/router.js#L293 fixes my issue, although I haven't tested other cases yet and not quite sure what is the purpose of this variable at all.

@RealJTG RealJTG changed the title Requirements are actually not required for generating URL in some cases Parameters without defaults are not required to generate URL in some cases Nov 14, 2019
@RealJTG
Copy link
Author

RealJTG commented Nov 15, 2019

optional = true was added here #81 (diff)

@tobias-93
Copy link
Collaborator

Hi @RealJTG, thanks for pointing out this issue.
It was actually introduced in 8298523#diff-8e1e018bfe2bd15df10d1864a922ff0cR64, the diff you indicated was just a rewrite of the same code.
It has to do with the definition of optional parameters in Symfony, of which more can be read here: https://symfony.com/doc/current/routing.html#optional-parameters.
The current implementation in this bundle does not comply with the requirements written there.
I think it is not doable to make it comply with that, since defaults for parameters can be implemented by giving a default value in the PHP method definition when using annotations.
These default values are not available to the tools generating the routing export, so instead of that it is implemented by looking for the first defined variable routing part (or text part), after which each variable is required to be defined since otherwise the route will be invalid.
Any routes not sticking to this assumption will be invalid for that place, which will lead to interesting behavior.

Do you have any suggestions on how to implement this in a better way?
We cannot simply assume every parameter to be defined or have a default in the export, since that is not required by Symfony.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants