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

Command can be disabled on mutualized servers #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Command can be disabled on mutualized servers #4

wants to merge 3 commits into from

Conversation

jerrywham
Copy link

No description provided.

For mutualized servers, command can be disabled. So component checked
if it is available.
@lGuillaume124
Copy link
Contributor

Hi,

Thank you for your contribution ! We were not expecting so much success and we need to organise the repository and its branches before accept your contribution.

@MightyCreak
Copy link
Contributor

I'm not sure about the function name is_shell_exec_available which just seems to check if shell_exec is available, but it also run the given command. This is pretty misleading.

It might be better to have a ShellExec component which would have an exec method that will first verify (in the constructor?) if shell_exec availability before executing the given command.

@MightyCreak
Copy link
Contributor

About the exif_read_data commit (fd0ded2).

First, it mixes CheckCmd changes and the actual bug. The CheckCmd changes should be in your first commit (f488700).

I don't think this bugfix should be in this PR, but should have it's own PR. It would be cleaner.

Finally, the exif_read_data documentation doesn't mention handling gif images, but you added it in the extension array.

@jerrywham
Copy link
Author

@MightyCreak : I agree with all what you said. Sorry for these mistakes.

@MightyCreak
Copy link
Contributor

No worries ;)

I'm just helping the Sonerezh devs, hoping they'll find time to merge all these interesting PR.

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.

3 participants