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

Improve error message when uploading image without correct extension or mimetype #2211

Merged

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Nov 12, 2021

Subject

I am targeting this branch, because this is a bugfix.

Closes #2208 .

Changelog

### Fixed
- Fixed call to `strlen()` with possible null value.
- Improved error message for uploaded images that do not have an allowed extension or mime type.

@jordisala1991
Copy link
Member Author

@sarim let's see what github actions says, but on my computer I couldn't make this test fail.

VincentLanglet
VincentLanglet previously approved these changes Nov 12, 2021
@sarim
Copy link

sarim commented Nov 12, 2021

@jordisala1991 I think your test are passing because resizer / provider etc.. service are mocked in that test file, not real services.

Here I just grabbed a random cat pic from imgur and can reproduce the error.

image

@jordisala1991
Copy link
Member Author

jordisala1991 commented Nov 13, 2021

You are right @sarim, but I have just changed the test to not mock anything related to size calculations. Now it is not mocked. I have left the GD implementation of imagine because that's what we are installing on github-actions. But I have tested with: Gd, imagick and gmagick. Those 3 implementations works and produce the same result.

We can merge this test because at the end it improves coverage, but as far as I can tell, if there is any issue you need to look at Imagine library, not here.

I left this as patch because I have added a fix on a strlen call, but it is only related to stop mocking things we don't own rather than the original bug reported.

@jordisala1991 jordisala1991 marked this pull request as ready for review November 13, 2021 07:13
@jordisala1991 jordisala1991 requested a review from a team November 13, 2021 07:15
@jordisala1991 jordisala1991 force-pushed the hotfix/test-without-extension branch 2 times, most recently from 74c2978 to 43008a8 Compare November 13, 2021 07:26
@jordisala1991
Copy link
Member Author

Now that I take a look at the add media command, do you have the problem only when using the command? did you tried to use the admin panel to upload that image?

I think the problem might be because of the usage of this command.

@jordisala1991 jordisala1991 force-pushed the hotfix/test-without-extension branch from 43008a8 to fb7c89e Compare November 13, 2021 08:11
@jordisala1991 jordisala1991 marked this pull request as draft November 13, 2021 08:11
@jordisala1991
Copy link
Member Author

Finally, it is related directly to the command and how all the flow executes. Now github actions fails with the same error reported.

@jordisala1991 jordisala1991 force-pushed the hotfix/test-without-extension branch from fb7c89e to 7288a03 Compare November 13, 2021 08:12
@sarim
Copy link

sarim commented Nov 13, 2021

You kinda lost me in this complex testing technology. If you would've just run the command from any repo (for example I showed screen from sonata sandbox) you would've encountered the problem. The problem is simple, as I described in my original issue. The problem was NOT in sonata v2. I upgraded a symfony2+sonata2 project to symfony 5 + sonata 4 and encountered this problem.

Let me reiterate : From Usage doc

$media = new Media();
$media->setBinaryContent(PATH TO IMAGE FILE);
$media->setContext('default');
$media->setProviderName('sonata.media.provider.image');

/** @var Sonata\MediaBundle\Model\MediaManagerInterface $mediaManager */
$mediaManager->save($media);

This is the way to create a new media. I was creating image using this code after a form submit, as you know, php saves uploaded files in /tmp/ABCD_RANDOM_CHARACTERS_WITHOUT_EXTENSION. So thats the path I'm passing to $media->setBinaryContent. Which used to work fine in sonata v2, but now it does not.

Now the error is simple and easy to reproduce, as the media add command uses the same code to create media, same error happens there.

I brought up the add command because it is very to easy to check. You just cd to any sonata project and run the command. The main issue is you can't add media using the referenced code.

@jordisala1991
Copy link
Member Author

I need more time to investigate. So far my findings are that it is harder to reproduce on a unit test, but I can clearly reproduce on the functional test, running the command directly.

Seems related to a missing step during the upload process because I can updateMetadata on a media and it works fine, with or without file extension.

The process to fix this requires a test to ensure no regressions after, so thats why is not as simple as opening a project and running the command you provided.

@sarim
Copy link

sarim commented Nov 13, 2021

First we want to verify if a bug really exist? or a user error. To answer that yes/no question you can run the command to verify, that was my intention. From my POV it looked like you were trying to reproduce it using complex testing technology while running a simple command would do. I mean the issue is still marked unconfirmed.

The process to fix this requires a test to ensure no regressions after, so thats why is not as simple as opening a project and running the command you provided

Ah, I understand.

@jordisala1991
Copy link
Member Author

Until right now I couldn't confirm it exists thats why it does have the label.

@jordisala1991 jordisala1991 force-pushed the hotfix/test-without-extension branch 2 times, most recently from 036103e to 9c30b7c Compare November 13, 2021 16:47
@jordisala1991 jordisala1991 changed the title Add test for images without extension Improve error message when uploading image without correct extension or mimetype Nov 13, 2021
@jordisala1991 jordisala1991 marked this pull request as ready for review November 13, 2021 16:50
@jordisala1991
Copy link
Member Author

The focus of the PR has changed a bit after finding why it doesn't work without extensions, more here: #2208

@jordisala1991 jordisala1991 force-pushed the hotfix/test-without-extension branch from 9c30b7c to ac58654 Compare November 13, 2021 17:04
VincentLanglet
VincentLanglet previously approved these changes Nov 14, 2021
@phansys phansys merged commit db1f720 into sonata-project:3.x Nov 15, 2021
@phansys
Copy link
Member

phansys commented Nov 15, 2021

Thank you @jordisala1991!

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

Successfully merging this pull request may close these issues.

4 participants