Skip to content

Added a check to prevent non-CLI calls from failing. #9

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

Merged
merged 6 commits into from
May 23, 2019

Conversation

im-bryan
Copy link
Contributor

When using this package in a script that isn't called through the CLI, there is a big chance that the functions defined in the PCNTL package aren't available, because of obvious security implications.

Because of the fact that the pcntl_signal closure is set in the constructor of the RedisSimpleLock, this class is unusable when used through different measure than the PHP CLI.

This PR makes sure that the closure is only attached when the CLI is used.

@mathroc
Copy link
Member

mathroc commented May 22, 2019

thx @im-bryan for this PR, this is something that was already requested and I didn't have time to work on ( see #6 )

@im-bryan
Copy link
Contributor Author

@mathroc I changed some stuff around. You can now explicitly ignore specific SAPIs when instantiating the RedisSimpleLock. I've also included tests to prove the feature is working as intended.

@mathroc
Copy link
Member

mathroc commented May 23, 2019

awesome thank you !

it looks ready for me, let me know if it's ok to merge for you and I 'll do it and release a new version

@im-bryan
Copy link
Contributor Author

@mathroc I don't have anything left to do. So I'd be very happy if you could merge it in.
The only thing I'm not happy with is that pcntl_signal_get_handler was introduced in version 7.1 of PHP so the tests only actually test this feature through PHPUnit in PHP 7.1 and up. AFAIK I cannot really get information about registered signals without this function, so PHP7.0 and down.

@mathroc
Copy link
Member

mathroc commented May 23, 2019

that's fine with me, it's not fully tested on <=7.0 but those versions of php are not supported anymore anyway ( https://www.php.net/supported-versions.php )

and that's relatively straightforward so testing it on 7.1+ is most likely enough

@mathroc mathroc merged commit 11250f4 into texthtml:master May 23, 2019
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.

2 participants