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

Check file_uploads ini directive for allowed uploads #5973

Closed
wants to merge 2 commits into from
Closed

Check file_uploads ini directive for allowed uploads #5973

wants to merge 2 commits into from

Conversation

ozdemirburak
Copy link
Contributor

Hoping that it fixes the issue #5963.

There may be one possible issue with testing where the user is not allowed to change the file_uploads ini directive.

That's why added the code block below to the tests.

if (ini_set('file_uploads', '0') !== false) {
    ...
}

@GwendolenLynch
Copy link
Contributor

@CarsonF can you 👍 / 👎 … I know we're aiming to nuke the permission class, but short term this looks to get us something

@bobdenotter
Copy link
Member

Bumping @CarsonF 👊

@GwendolenLynch
Copy link
Contributor

@bobdenotter just flagging, if you're near the green button, I want to send this to a lower version branch than master

Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

I would rather use an Exception class/subclass from Bolt\Filesystem or Symfony\Filesystem. This class is kind of an odd ball right now. It will probably be removed within a few versions.

I also don't love an exception being thrown from a "check" method that returns a boolean. I would expect it just to return false. But that leaves us with no information...

@ozdemirburak
Copy link
Contributor Author

@CarsonF updated, couldn't find any specific exception but IOException.

@CarsonF
Copy link
Member

CarsonF commented Nov 10, 2016

@GawainLynch What do you think? I'm thinking a subclass should be created for this. It doesn't really feel like an IO exception. Although the subclass could extend IO.

@GwendolenLynch
Copy link
Contributor

I'm thinking a subclass should be created for this. It doesn't really feel like an IO exception. Although the subclass could extend IO.

Well right this second it is something of a moot point, as we're not catching the exception, and providing feedback. This is into the editor zone of UX after all.

But yeah, a ThisIsNotTheIniSettingYouAreLookingForException() might be a bit more useful in catching and displaying something in a clean way. The question becomes, what?

@GwendolenLynch
Copy link
Contributor

Merged into release/3.3 via 83fa3db

Thanks for your patience @ozdemirburak 👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants