Fix #79969: Wrong error message in strict mode in ReflectionMethod::_construct #5995
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,7 @@ echo "Done\n"; | |
| ?> | ||
| --EXPECT-- | ||
| string(20) "Invalid method name " | ||
| string(66) "The parameter class is expected to be either a string or an object" | ||
| string(21) "Class does not exist" | ||
| string(22) "Class a does not exist" | ||
| string(21) "Class does not exist" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,20 +7,40 @@ Steve Seear <[email protected]> | |
| <?php | ||
|
|
||
| try { | ||
| new ReflectionMethod(); | ||
| } catch (Throwable $e) { | ||
| var_dump($e->getMessage()); | ||
| } | ||
|
|
||
| try { | ||
| new ReflectionMethod('a', 'b', 'c'); | ||
| } catch (Throwable $e) { | ||
| var_dump($e->getMessage()); | ||
| } | ||
|
|
||
|
|
||
| class C { | ||
| public function f() {} | ||
| } | ||
|
|
||
| try { | ||
| new ReflectionMethod(new C); | ||
| } catch (Throwable $e) { | ||
| var_dump($e->getMessage()); | ||
| } | ||
|
|
||
| try { | ||
| new ReflectionMethod(new C, null); | ||
| } catch (Throwable $e) { | ||
| var_dump($e->getMessage()); | ||
| } | ||
|
|
||
| try { | ||
| new ReflectionMethod(new C, ""); | ||
| } catch (Throwable $e) { | ||
| var_dump($e->getMessage()); | ||
| } | ||
|
|
||
| $rm = new ReflectionMethod('C', 'f'); | ||
|
|
||
| var_dump($rm->isFinal(1)); | ||
|
|
@@ -43,8 +63,11 @@ var_dump($rm->getName(1)); | |
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| string(69) "ReflectionMethod::__construct() expects at least 1 parameter, 0 given" | ||
| string(69) "ReflectionMethod::__construct() expects at most 2 parameters, 3 given" | ||
| string(76) "ReflectionMethod::__construct() expects parameter 2 to be string, null given" | ||
| string(76) "ReflectionMethod::__construct() expects parameter 2 to be string, null given" | ||
| string(27) "Method C::() does not exist" | ||
|
|
||
| Warning: ReflectionMethod::isFinal() expects exactly 0 parameters, 1 given in %s on line %d | ||
| NULL | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,14 +65,14 @@ try { | |
| ?> | ||
| --EXPECTF-- | ||
| Wrong type of argument (bool): | ||
| ReflectionException: The parameter class is expected to be either a string or an object in %s:%d | ||
| Stack trace: | ||
| #0 %s ReflectionMethod->__construct(true) | ||
| #1 {main} | ||
| Wrong type of argument (int): | ||
| ReflectionException: The parameter class is expected to be either a string or an object in %s:%d | ||
| Stack trace: | ||
| #0 %s ReflectionMethod->__construct(3) | ||
| #1 {main} | ||
| Wrong type of argument (bool, string): | ||
| ReflectionException: The parameter class is expected to be either a string or an object in %s | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,21 +12,22 @@ class TestClass | |
| } | ||
| } | ||
|
|
||
| echo "Too few arguments:\n"; | ||
|
|
||
| try { | ||
| $methodInfo = new ReflectionMethod(); | ||
| } catch (Throwable $e) { | ||
| var_dump($e->getMessage()); | ||
| } | ||
|
|
||
| echo "\nToo many arguments:\n"; | ||
|
|
||
| try { | ||
| $methodInfo = new ReflectionMethod("TestClass", "foo", true); | ||
| } catch (Throwable $e) { | ||
| var_dump($e->getMessage()); | ||
| } | ||
|
|
||
| try { | ||
| //invalid class | ||
| $methodInfo = new ReflectionMethod("InvalidClassName", "foo"); | ||
|
|
@@ -50,12 +51,12 @@ try{ | |
| } | ||
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| Too few arguments: | ||
| string(69) "ReflectionMethod::__construct() expects at least 1 parameter, 0 given" | ||
|
|
||
| Too many arguments: | ||
| string(69) "ReflectionMethod::__construct() expects at most 2 parameters, 3 given" | ||
| Ok - Class InvalidClassName does not exist | ||
| Ok - The parameter class is expected to be either a string or an object | ||
| Ok - ReflectionMethod::__construct() expects parameter 2 to be string, array given | ||
|
There was a problem hiding this comment. Nit: missing new line before EOF |
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| --TEST-- | ||
| Bug #79969: ReflectionMethod::__construct throws incorrect exception when 2nd parameter is int | ||
| --FILE-- | ||
| <?php | ||
| declare(strict_types=1); | ||
|
|
||
| new ReflectionMethod('xxx', 1); | ||
| ?> | ||
| --EXPECTF-- | ||
| Fatal error: Uncaught TypeError: ReflectionMethod::__construct() expects parameter 2 to be string, int given in %s | ||
| Stack trace: | ||
| #0 %s(%d): ReflectionMethod->__construct('xxx', 1) | ||
| #1 {main} | ||
| thrown in %s on line %d |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we'll need @cmb69 's approval for making the second parameter nullable for PHP 7.3. But it should absolutely work for PHP 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a strict no from me, but I'd rather not change the signature in a stable version. @derickr what do you think about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Probably adding the
!modifier only makes sense for PHP 8.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to not complicate it further it would be better if I revert the code back to the first commit in this PR, and then apply this feedback. Then, for PHP 8, a different solution like having a nullable 2nd parameter, could be applied.
However, I am not sure if this is desirable and, in case it is, about the procedure to make a change in different PHP versions.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's also a sensible idea. We could also ask from ourselves if this is really needed to be fixed in PHP 7 at all? Personally, I'd only fix it in 8, but ultimately, it's Christoph's and Derick's call :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that fixing the erroneous error message for PHP-7.3 makes sense (given there is no change in the signature, i.e. second parameter is not nullable). After that has been merged into PHP-7.4 and the "master" branch, another PR removing the function overload (i.e. second parameter made nullable) could follow up.