SonataMediaBundle: [Bug][Routing] "format" treated as part of media "id"
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:
<route id="sonata_media_download" path="/download/{id}/{format}">
<default key="_controller">SonataMediaBundle:Media:download</default>
<default key="format">reference</default>
<requirement key="id">.*</requirement>
</route>
<route id="sonata_media_gallery_view" path="/view/{id}">
<default key="_controller">SonataMediaBundle:Gallery:view</default>
<requirement key="id">.*</requirement>
</route>
This 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.
The 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.
A specific example of this bug in action:
For 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.
Am 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.
About this issue
- Original URL
- State: closed
- Created 9 years ago
- Comments: 45 (41 by maintainers)
i don’t think expecting the request in the controller action is deprecated. see also https://symfony.com/doc/current/controller.html#the-request-object-as-a-controller-argument . what is deprecated in symfony 2 and no longer possible in 3 is using the
requestservice, because that created a ton of problems with temporality and subrequests.my gut feeling would have been to put the format before the id and use original by default. but if people are expected to call the path/generate function directly, that is indeed a BC break. i actually can’t think of a good argument why the format should not be in the query string. so my vote goes for query string.
we could keep a differently named BC route definition with the format at the end of the path, to not mess up on deployment with cached pages and such. and then i think this can be done completely BC except for when people extended the controller. we could even add a new controller method instead of changing the existing, that calls the old method with the format extracted from the request, to avoid a BC break alltogether.
Oh, good to know. I guess your solution should be quite fine, in that case.
formatis supposed to map to a size (‘big’, ‘small’, ‘reference’), not to a file extension.I think both solution are quite fine, and that we should let @NeuralClone decide what he thinks is best, since he probably has more context in memory.
Ok, great! @sonata-project/contributors , can you please help us take a decision? What do you think would be best?
I thought of it, but didn’t propose it because I see more cons:
formatparameter mandatory is a BC-break for people using the route directly.bigformat, which means that putting theformatfirst isn’t very natural either.Additionally, I don’t know why you guys think urls with a query string are less pretty. I’d says that paths are great for expressing a clear hierarchy, like
/folder/subfolder/file, but when you have parameters that are more like tags you put on a given resource, using `file?size=big&type=grayscale&ratio=16-9’ feels a bit more logical. You want to get a slightly different version of the same resource .With that approach :
path()uses the query string for anything that does not appear in the url pattern, so no change in url generation eitherSo the only con IMO would be the need to add a
$requestargument to the controller (no need to access the request stack, right? This would be a BC-break, but a very small one : it only breaks controllers extending the media controller.Damn. Back to square one then.
That would mean using
findByUuidjust for this manager, right? What about my solution, using/{phpcr-seo-friendly-id-with-forward-slashes}?format={format}? It won’t require complicated logic for this linei would not be aware of a decision to deprecate the phpcr-odm integration here. what we atm don’t maintain anymore is the https://github.com/symfony-cmf/media-bundle which builds on top of the sonata media bundle.
in phpcr-odm, document ids indeed are paths with forward slashes. the alternative would be to use the uuid of the documents which is a uuid v4. this will make for less nice urls (seo people love to have nice image urls with their words in it) but we could do that. we could also allow random words to come after the format, something like /{id}/{format}/{random-seo-blah}.png. if we need a major version bump for that, go for it. if you want to remove phpcr-odm support, so be it - i am glad to give some input if we can keep it in, but can’t commit to do a lot of work to maintain the support.
Yeah, that’s exactly what was confusing me. I was operating under the assumption that
formatshould be getting filled when it actually isn’t. What makes it more confusing is thatformatgets filled when using the view route. Or it can be in certain situations anyway.Unless paths are hardcoded in someone’s controller or templates, and that’s a very bad practice, I’m leaning towards it not being a BC-break to move
formatto the query string. The functionality and signature of the controller should remain the same with that change.Yeah, that’s a very good point. Whatever the case, it’s no longer a problem. 😃
And yes, I’ll definitely add automated tests for that. I’ll try to get a pull request up in the next day or so.
Yeah, I believe that’s correct.
The
.+requirement on format (in addition to the existing requirement,.*, on id) appears to work. I’m going to do some more tests to make sure things are working as they should but right now it’s looking like a good fix. 👍Not so sure about that after all. I think “full paths” as referred to in a88082e. So maybe the solution would be to add a constraint on format instead, ensuring it can’t be empty, because that’s what is happening when the bug happens, right? id eats format when it shouldn’t.
Long story short, if I understood correctly, we want
download/some/path/jsonto givesome/pathas an id andjsonas a format, so my first suggestion is obviously wrong. So maybe try to add.+as a requirement for format.@greg0ire Not stupid at all. 😃 For some reason I didn’t get a notification about your first follow-up comment. It wasn’t my intention to ignore you. Sorry about that!
I’ll give that suggestion a try.
@core23 Sure, I can work on that. Which branch should I create the PR on? 3.x?