8000 Discourage libevent on PHP 7 by cboden · Pull Request #62 · reactphp/event-loop · GitHub
[go: up one dir, main page]
Skip to content

Discourage libevent on PHP 7 #62

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

Merged
merged 2 commits into from
Oct 13, 2017
Merged

Discourage libevent on PHP 7 #62

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bda850f
Discourage libevent on PHP 7
cboden Nov 17, 2016
f1c4746
Remove warning on invalid version mismatch
cboden Oct 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Discourage libevent on PHP 7
  • Loading branch information
@cboden @clue
cboden authored and clue committed Oct 13, 2017
commit bda850fdb4d67b0a5d316e36cdd01ac3842345d9
7 changes: 4 additions & 3 deletions src/Factory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ class Factory
public static function create()
{
// @codeCoverageIgnoreStart
if (function_exists('event_base_new')) {
return new LibEventLoop();
} elseif (class_exists('libev\EventLoop', false)) {
if (class_exists('libev\EventLoop', false)) {
return new LibEvLoop;
} elseif (class_exists('EventBase', false)) {
return new ExtEventLoop;
} elseif (function_exists('event_base_new') && PHP_VERSION_ID < 70000) {
// only use ext-libevent on PHP < 7 for now
return new LibEventLoop();
Copy link
Member

Choose a reason for hiding this comment

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

Makes perfect sense to lower priority here and do not pick this extension on PHP 7+ at all 👍

}

return new StreamSelectLoop();
Expand Down
4 changes: 4 additions & 0 deletions src/LibEventLoop.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ class LibEventLoop implements LoopInterface

public function __construct()
{
if (version_compare(PHP_VERSION, '7.0.0') >= 0) {
trigger_error('The libevent extension has not yet been updated for PHP 7, it is recommended you use a different loop', E_USER_WARNING);
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of this tbqh 👎

With the above check in place we do not automatically create the LibEventLoop from our Factory. Afaict this is what most people use, so this should resolve this for the most part?

This means that if this constructor is invoked, it's explicitly been called. IMHO this is already advanced usage and we should not trigger a warning in this case.

Otherwise, how could a user possibly ignore this warning? Using @-suppression would result in zero output if the extension can not be loaded all.

Also, what happens when the extension will be updated to support PHP 7 again? We can't really detect this until we provide an update, in which case the above warning would be superfluous.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the user has upgraded to PHP7 and somehow kept the extension installed, from the reports I've received, it's going to segfault. The trigger_error only issues a warning, the object will still be instantiated an be able to be used.

Also, what happens when the extension will be updated to support PHP 7 again?

It looks like that extension isn't going to be...but if it does we'll have to quickly update this to support it. Even if this happens the compare only issues a warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the consensus is still a 👎 on this I'm good to remove this check in order to get the updated Factory order into the next release.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we had a consensus here, it was basically just my point of view :-) But if this error is removed, I'm all for getting this in! 👍


$this->eventBase = event_base_new();
$this->futureTickQueue = new FutureTickQueue();
$this->timerEvents = new SplObjectStorage();
Expand Down
0