Fixed #74035 - ReflectionClass::newInstance param set as optional #2363
Conversation
ext/reflection/php_reflection.c
Outdated
| @@ -6273,7 +6273,7 @@ ZEND_BEGIN_ARG_INFO(arginfo_reflection_class_isInstance, 0) | |||
| ZEND_ARG_INFO(0, object) | |||
| ZEND_END_ARG_INFO() | |||
|
|
|||
| ZEND_BEGIN_ARG_INFO(arginfo_reflection_class_newInstance, 0) | |||
| ZEND_BEGIN_ARG_INFO_EX(arginfo_reflection_class_newInstance, 0, 0, 0) | |||
| ZEND_ARG_INFO(0, args) | |||
There was a problem hiding this comment.
While at it, this should be specified as a variadic argument.
There was a problem hiding this comment.
@nikic Good catch! thanks! fixed now.
andrewnester
force-pushed
the
74035-reflection
branch
from
13006de
to
4b045a7
Compare
|
Merged via 9ffc6ca, thanks! |
|
This commit appears to have broken doctrine-common's static reflection parser - ie. \Doctrine\Common\Reflection\StaticReflectionClass::newInstance() Drupal 8 tests against php 7.0-dev and 7.1-dev and we're seeing lots of errors like: We're tracking the issue in https://www.drupal.org/node/2850639 |
|
Aww, crap. Looks like we can't make this parameter optional without breaking BC. I will revert this from 7.0 and 7.1 and leave it in master only. |
|
@nikic I think it might have only been the change to specify it being variadic that looks like it was an addition to the patch on review. |
|
@alexpott Unfortunately both are breaking changes, because making an optional parameter required during inheritance is not legal. |
|
Change reverted by eb1373e in 7.0 and 7.1. The change is still in master, so code still needs to be adjusted to use the correct signature at some point. |
|
Wow, sorry! Will be a good lesson for me in future to check if my changes break BC |
|
@nikic this is about to break things in PHP7.2. Is this change allowed in a minor PHP version? |
|
Ping @sgolemon @remicollet, this has NOT been reverted for 7.2 and still is a BC break: https://3v4l.org/7igHL |
|
There's already an open bug report for this: https://bugs.php.net/bug.php?id=74292 |
|
PR against PHP-7.2 reverting this: #2893 |
Fix for https://bugs.php.net/bug.php?id=74035