8000 [Thumbnail] Fix thumbnail URL for reference format by NeuralClone · Pull Request #787 · sonata-project/SonataMediaBundle · GitHub
[go: up one dir, main page]
Skip to content

[Thumbnail] Fix thumbnail URL for reference format #787

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
wants to merge 1 commit into from
Closed

[Thumbnail] Fix thumbnail URL for reference format #787

wants to merge 1 commit into from

Conversation

NeuralClone
Copy link
Contributor

Fixes a bug in Sonata\MediaBundle\Thumbnail\FormatThumbnail::generatePrivateUrl() that causes an incorrect file name to be returned when "reference" format is requested.

This fix is a follow-up to #232 and #277. #277 fixed the issue in FileProvider but left a similar bug in ImageProvider. Credit should go to @tiagojsag for bringing up the original problem and for suggesting a similar fix.

This bug impacts X-Sendfile and X-Accel-Redirect send modes.

soullivaneuh
soullivaneuh reviewed Jun 13, 2015
View reviewed changes
Provider/ImageProvider.php
return $this->thumbnail->generatePrivateUrl($this, $media, $format);
if ($format == 'reference') {
return $this->getReferenceImage($media);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else is not necessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, yes, you're right. I'll update this. :)

@phansys
Copy link
Member
phansys commented Jun 15, 2015

@NeuralClone, could you add some tests?

@NeuralClone
Copy link
Contributor Author

Yeah, I can try to add some tests. I'm not sure when I'll have time to get to that, though.

@NeuralClone
Copy link
Contributor Author

I've updated this pull request to include some additional tests for ImageProvider. The tests all pass. I didn't switch over to Yoda conditions for the time being but I'll gladly make those changes if that's desired.

Without the fix in ImageProvider::generatePrivateUrl(), the first test I added fails as the function returns default/0011/24/thumb_1023456_reference.png instead of the reference URL.

@phansys
Copy link
Member
phansys commented Jun 22, 2015

Ref. for Yoda: symfony/symfony-docs#5402

@soullivaneuh
Copy link
Member

@NeuralClone Please update only your modification with Yoda comparison.

Since a fixer will be merged on next version of php-cs-fixer, code will be automatically fixed later anyway. 👍

@NeuralClone
Copy link
Contributor Author

Ok, sounds good! I thought that made more sense. Not sure why I didn't just go with that in the first place. Long weekend I guess? :)

Anyway I'll revert this to just my fix plus the tests I added later today.

@NeuralClone
Copy link
Contributor Author

This is now back to just my fix and the two tests I added. Sorry for the earlier deviation from that!

@NeuralClone
Copy link
Contributor Author

I was just wondering if there was any additional feedback on this pull request. It's been over a month since I originally submitted this and all of the feedback I've received has been about coding styles and standards.

While I agree that that stuff is important, it ultimately isn't relevant to the bug this pull request fixes. Nor is it particularly helpful. X-Sendfile is currently broken in this bundle when dealing with images, which makes the bundle mostly worthless when dealing with large image files. Sending large files through HTTP with PHP is rather slow. It doesn't make sense to use that mode when other, superior modes are available for use. And regardless of that, the bundle is supposed to support X-Sendfile.

I'm willing to provide additional tests if more are required to verify the existence of this bug and that this pull request fixes it. But since I haven't received any useful feedback since I submitted this, I have no idea where this stands and what the intentions are regarding this bug. Will this fix eventually be merged? If so, any estimate on when that might be? Or is support for X-Sendfile being removed in a later version of the bundle, which would render this fix pointless?

Thanks!

@phansys
Copy link
Member
phansys commented Jul 17, 2015

I think this fix doesn't only affects X-Sendfile; moreover, this should be propagated to other methods IMO. By instance, what happen by instance if you call Sonata\MediaBundle\Thumbnail\FormatThumbnail::generatePrivateUrl() passing "reference" as 3rd argument?

@NeuralClone
Copy link
Contributor Author

I've updated this pull request to simplify the fix and address @phansys's very valid points. The fix itself has been moved into FormatThumbnail::generatePrivateUrl() as that is the source of the problem. Adding the fix to this function, however, makes the function identical to FormatThumbnail::generatePublicUrl(). I decided to make it so the functions aren't reliant on each other. The end result is some code duplication. I don't find this to be ideal but I think it makes the code easier to modify. The obvious way to remove this duplication is to have FormatThumbnail::generatePrivateUrl() call FormatThumbnail::generatePublicUrl().

@NeuralClone NeuralClone changed the title Fix for X-Sendfile issue in ImageProvider [Thumbnail] Fix thumbnail URL for reference format Aug 5, 2015
@NeuralClone
Copy link
Contributor Author

@rande @soullivaneuh

Nothing? It'd be really nice to get this bug fixed. If my approach to fixing it isn't the desired approach, it would be great to know that. I'll gladly make whatever changes are necessary to get this merged. But without much feedback or any update on the status of this in 3 months, I'm starting to feel that fixing bugs isn't on the list of priorities for this project. That's both baffling and disappointing if that's the case.

I really do not understand why this continues to be ignored. This is a bug that prevents a major feature from working and it has a trivial fix that was suggested over 2 years ago.

@soullivaneuh
Copy link
Member

@NeuralClone I'm sorry but I'm not the best to review this PR on this bundle.

I'm sure @rande is not ignoring your PR but have a lot of work to do, just be patient. 👍

Just a question: Is this bug exists also on 2.3? If yes, could you reopen a PR on this branch instead of master? Thanks.

@NeuralClone
Copy link
Contributor Author

@soullivaneuh Thanks for the response! I appreciate it. :) I wasn't quite sure who was best to bring this to the attention of.

And yes, I'm pretty sure this exists in 2.3 (the bug has existed in some form for quite a while) but I'll double check and create a new PR on that branch if appropriate.

@soullivaneuh
Copy link
Member

I'll double check and create a new PR on that branch if appropriate.

Thanks. 👍

@NeuralClone NeuralClone mentioned this pull request Sep 17, 2015
Merged
@NeuralClone NeuralClone deleted the image_provider_xsendfile_fix branch September 17, 2015 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NeuralClone @phansys @soullivaneuh
0