FriendsOfSymfony / FOSJsRoutingBundle

A pretty nice way to expose your Symfony routing to client applications.
1.48k stars 261 forks source link

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

Open RealJTG opened 4 years ago

RealJTG commented 4 years ago

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 commented 4 years ago

optional = true was added here https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/pull/81#discussion-diff-3648557

tobias-93 commented 4 years ago

Hi @RealJTG, thanks for pointing out this issue. It was actually introduced in https://github.com/friendsofsymfony/FOSJsRoutingBundle/commit/8298523387158aa11fbfae2893d6b17866321111#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.