{"action":"created","issue":{"url":"https://api.github.com/repos/sonata-project/SonataMediaBundle/issues/799","repository_url":"https://api.github.com/repos/sonata-project/SonataMediaBundle","labels_url":"https://api.github.com/repos/sonata-project/SonataMediaBundle/issues/799/labels{/name}","comments_url":"https://api.github.com/repos/sonata-project/SonataMediaBundle/issues/799/comments","events_url":"https://api.github.com/repos/sonata-project/SonataMediaBundle/issues/799/events","html_url":"https://github.com/sonata-project/SonataMediaBundle/issues/799","id":90466801,"number":799,"title":"[Bug][Routing] \"format\" treated as part of media \"id\"","user":{"login":"NeuralClone","id":678221,"avatar_url":"https://avatars.githubusercontent.com/u/678221?v=3","gravatar_id":"","url":"https://api.github.com/users/NeuralClone","html_url":"https://github.com/NeuralClone","followers_url":"https://api.github.com/users/NeuralClone/followers","following_url":"https://api.github.com/users/NeuralClone/following{/other_user}","gists_url":"https://api.github.com/users/NeuralClone/gists{/gist_id}","starred_url":"https://api.github.com/users/NeuralClone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/NeuralClone/subscriptions","organizations_url":"https://api.github.com/users/NeuralClone/orgs","repos_url":"https://api.github.com/users/NeuralClone/repos","events_url":"https://api.github.com/users/NeuralClone/events{/privacy}","received_events_url":"https://api.github.com/users/NeuralClone/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/sonata-project/SonataMediaBundle/labels/bug","name":"bug","color":"fc2929"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":39,"created_at":"2015-06-23T19:06:02Z","updated_at":"2016-10-15T19:22:43Z","closed_at":null,"body":"When interpreting download routes for media, the `format` parameter is currently treated as part of the `id` parameter due to the requirement placed on ids:\r\n\r\n```xml\r\n<route id=\"sonata_media_download\" path=\"/download/{id}/{format}\">\r\n    <default key=\"_controller\">SonataMediaBundle:Media:download</default>\r\n    <default key=\"format\">reference</default>\r\n    <requirement key=\"id\">.*</requirement>\r\n</route>\r\n```\r\n```xml\r\n<route id=\"sonata_media_gallery_view\" path=\"/view/{id}\">\r\n    <default key=\"_controller\">SonataMediaBundle:Gallery:view</default>\r\n    <requirement key=\"id\">.*</requirement>\r\n</route>\r\n```\r\nThis is less of an issue with galleries since an incorrect route will cause a `NotFoundHttpException` to be thrown as expected (although I find this lax requirement rather odd). However, this becomes problematic when attempting to download media when a format other than reference is specified because _valid_ routes will also cause a `NotFoundHttpException` exception to be thrown. Given this undesirable and unexpected behavior, this seems like a bug to me.\r\n\r\nThe rather lax requirement on the `id` parameter seems a bit strange since media and gallery ids are defined as integers in their respective model mappings. Changing the requirements on `id` to `\\d+` in both routes fixes this problem. It's easy enough to override the default MediaBundle routes in the ApplicationSonataMediaBundle that gets generated but that shouldn't be necessary to get those routes to work. This change also shouldn't be a BC break (as far as I can tell) unless people have written code to rely on this behavior.\r\n\r\nA specific example of this bug in action:\r\n\r\nFor a file with a media id of 100 and a format big (default context), the following route should be generated if requesting to download it: `/<some prefix>/download/100/default_big`. Instead of `100` being interpreted as the `id` and `default_big` being interpreted as the `format`, `100/default_big` is treated as the `id`, which is obviously incorrect.\r\n\r\nAm I correct in assuming that this is not the desired behavior? If this *is* the desired behavior, then it seems to render the `format` parameter completely pointless without additional code to interpret the `id` and `format` parameters, and that would seem like a rather roundabout way to deal with the parameters when the Routing component can handle most of the heavy lifting."},"comment":{"url":"https://api.github.com/repos/sonata-project/SonataMediaBundle/issues/comments/254005363","html_url":"https://github.com/sonata-project/SonataMediaBundle/issues/799#issuecomment-254005363","issue_url":"https://api.github.com/repos/sonata-project/SonataMediaBundle/issues/799","id":254005363,"user":{"login":"NeuralClone","id":678221,"avatar_url":"https://avatars.githubusercontent.com/u/678221?v=3","gravatar_id":"","url":"https://api.github.com/users/NeuralClone","html_url":"https://github.com/NeuralClone","followers_url":"https://api.github.com/users/NeuralClone/followers","following_url":"https://api.github.com/users/NeuralClone/following{/other_user}","gists_url":"https://api.github.com/users/NeuralClone/gists{/gist_id}","starred_url":"https://api.github.com/users/NeuralClone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/NeuralClone/subscriptions","organizations_url":"https://api.github.com/users/NeuralClone/orgs","repos_url":"https://api.github.com/users/NeuralClone/repos","events_url":"https://api.github.com/users/NeuralClone/events{/privacy}","received_events_url":"https://api.github.com/users/NeuralClone/received_events","type":"User","site_admin":false},"created_at":"2016-10-15T19:22:43Z","updated_at":"2016-10-15T19:22:43Z","body":"First of all, thanks for giving some input on this, @dbu. Some good alternative suggestions. On some level, it's unfortunate that PHPCR-ODM is still being supported because it complicates the fix.\r\n\r\nThis ended up being a bit longer than I intended it to be but I wanted give the full rationale for my suggested fix. Please bear with me. I am going somewhere with this. 😄 \r\n\r\nI think I may have come up with a solution that could work. It's a little different than what's been suggested so far and isn't perfect. Basically, why not flip the order of the parameters in the route configuration? I haven't fully tested this yet but I think it'd take the least amount of work to implement and is fairly intuitive since it works very similar to what we already have.\r\n\r\n```xml\r\n<route id=\"sonata_media_view\" path=\"/view/{format}/{id}\">\r\n    <default key=\"_controller\">SonataMediaBundle:Media:view</default>\r\n    <default key=\"format\">reference</default>\r\n    <requirement key=\"format\">[^/]+</requirement>\r\n    <requirement key=\"id\">.+</requirement>\r\n</route>\r\n\r\n<route id=\"sonata_media_download\" path=\"/download/{format}/{id}\">\r\n    <default key=\"_controller\">SonataMediaBundle:Media:download</default>\r\n    <default key=\"format\">reference</default>\r\n    <requirement key=\"format\">[^/]+</requirement>\r\n    <requirement key=\"id\">.+</requirement>\r\n</route>\r\n\r\n<route id=\"sonata_media_gallery_view\" path=\"/view/{id}\">\r\n    <default key=\"_controller\">SonataMediaBundle:Gallery:view</default>\r\n    <requirement key=\"id\">.+</requirement>\r\n</route>\r\n```\r\n\r\nRight now the problem is that `format` isn't being filled in all situations when it should be. It's impossible to fill it when PHPCR-ODM is in use. Since PHPCR-ODM is still being maintained, it isn't acceptable to not allow slashes in the `id` as they're critical to how it handles `id` values. I also don't think it makes sense to remove support for PHPCR-ODM just to fix this bug. Doing that would eliminate some of the flexibility of the bundle.\r\n\r\nAs part of my proposed solution, I'd also suggest tightening down the requirements slightly on both `format` and `id` to prevent empty parameters from appearing in `id` and `format` and slashes from appearing in `format`. Only preventing slashes in `format` is absolutely required for this to work, though.\r\n\r\nSome pros and cons for my proposed solution.\r\n\r\n**Pros**\r\n1) The Routing component maps route parameters to a controller's parameters by name, not by the order they appear. So the signature of the controllers could remain as they are now, which would promote BC.\r\n\r\n2) We could keep PHPCR-ODM support.\r\n\r\n3) `format` would always be filled.\r\n\r\n4) We could avoid using query strings (i.e., keep consistent \"pretty\" URLs).\r\n\r\n5) A very small change would be required in the way routes are generated and handled.\r\n\r\n6) No need to inject the RequestStack service or other services into controllers or helper services.\r\n\r\n7) Consistency between the way the `sonata_media_view` and `sonata_media_download` routes work. (Currently the view route doesn't properly support PHPCR-ODM `id` values that include slashes.)\r\n\r\n**Cons**\r\n1) The `format` parameter would become mandatory. Even if it's automatically filled in most situations by the bundle, it would have to appear in every path that's generated by the Routing component. For example: `/view/reference/my/phpcr/odm/id` instead of the current `/view/my/phpcr/odm/id`.\r\n\r\n2) The default format, `reference`, isn't very intuitive as part of URLs that users would be interacting with.\r\n\r\nI do have an idea for addressing the second con, though. We could create an alias for \"reference\" that would only be used in generated paths. I'd suggest \"original\" to represent the original format of the media. From a user's point of view, \"original\" would work like \"reference\". Internally, a few checks would need to be made but other than that the bundle would operate the same way it does now.\r\n\r\nIn other words, to users `/view/reference/my/phpcr/odm/id` would become `/view/original/my/phpcr/odm/id`. \"original\" would become an alias for \"reference\". Both would work through paths but only the version with \"original\" would appear to users. Functionally these two values would operate the same way.\r\n\r\nUsing \"reference\" in place of \"original\" shouldn't present any sort of security concern either since \"reference\" already works the way I'm suggesting \"original\" would. The one problem I see with making \"original\" a reserved `format` name is it could result in a BC-break if people have configured a format with that name. But that seems like a minor concern given the benefits these changes would provide."}}
{"actor":{"display_login":"NeuralClone"}}