Skip to content

Fix for PHP 7 fatal error on duplicate parameter names in closures #39

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

Closed

Conversation

ondrejmirtes
Copy link
Contributor

PHP 7 does not like duplicate parameter names: https://3v4l.org/usQhl

@ondrejmirtes ondrejmirtes force-pushed the php7-exteventloop-fix branch from cb7dc83 to 3bc352a Compare December 6, 2015 10:20
@ondrejmirtes
Copy link
Contributor Author

The build failed on PHP 5.5 because the composer.json requires PHPUnit version that requires PHP 5.6.

@WyriHaximus
Copy link
Member

That is interesting because the 5.4 build didn't fail. We don't specify phpunit in composer.json, the phpunit versions are supplied by travis

@clue
Copy link
Member

clue commented Dec 10, 2015

The changes LGTM and make perfect sense 👍

Though I'm a bit surprised, has #27 been solved yet? Afaict nobody has managed to build the required extensions, so this code will not actually run? (Only the StreamSelectLoop should work)

@dakota
Copy link

dakota commented Mar 1, 2016

Event 2.0 has been released with full PHP7 support. What is needed to get this to be merged?

@ondrejmirtes
Copy link
Contributor Author

We are already using PECL event 2.0 extension with PHP 7.0.3 with this patch and everything works fine.

@dakota
Copy link

dakota commented Mar 1, 2016

@ondrejmirtes I'm busy patching our local install. Wondering when this will end up in a event-loop release.

@ondrejmirtes
Copy link
Contributor Author

I will contact ReactPHP maintainers in order to ask them what can we do to bring ReactPHP up to speed.

@cebe
Copy link
Contributor

cebe commented Mar 1, 2016

#45 includes this fix and adds also tests for php 7.

@clue
Copy link
Member

clue commented Mar 1, 2016

Wondering when this will end up in a event-loop release.

At the moment the above changes are not tested in our CI setup. I assume this will be accepted once we come up with some installation instructions for the required extensions, so perhaps anybody may help with #27 or #45?

@cebe
Copy link
Contributor

cebe commented Mar 1, 2016

@clue #45 is ready to merge. libev and libevent do not support php7 so there is nothing to fix from our side atm.

@cboden
Copy link
Member

cboden commented Mar 8, 2016

Merged in via #45

@cboden cboden closed this Mar 8, 2016
@cboden cboden added this to the v0.4.2 milestone Mar 8, 2016
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.

6 participants