-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add method to ReflectionClass to instantiate with the same semantics as PDOStatement::fetchObject()
#19401
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
Open
mattdinthehouse
wants to merge
5
commits into
php:master
Choose a base branch
from
mattdinthehouse:feature/reflectionclass-newinstancefromdata
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+201
−2
Open
Add method to ReflectionClass to instantiate with the same semantics as PDOStatement::fetchObject()
#19401
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4066651
Add a `ReflectionClass::newInstanceFromData()` method that behaves th…
mattdinthehouse f33b64e
Update ext/reflection/php_reflection.stub.php
mattdinthehouse 0380b54
fix build now that we don't have a tentative return type
mattdinthehouse 87572e9
prevent users from instantiating internal classes from data because i…
mattdinthehouse 16b685d
there we go - now i know how stub files work
mattdinthehouse File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
90 changes: 90 additions & 0 deletions
90
ext/reflection/tests/ReflectionClass_newInstanceFromData_001.phpt
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,90 @@ | ||
--TEST-- | ||
ReflectionClass::newInstanceFromData | ||
--FILE-- | ||
<?php | ||
|
||
class A | ||
{ | ||
public int $a; | ||
public string $b; | ||
|
||
public function __construct($c, $d) | ||
{ | ||
echo "In constructor of class A\n"; | ||
} | ||
} | ||
|
||
class B | ||
{ | ||
public int $a; | ||
public string $b; | ||
} | ||
|
||
class C | ||
{ | ||
} | ||
|
||
#[\AllowDynamicProperties] | ||
class D | ||
{ | ||
} | ||
|
||
|
||
$rcA = new ReflectionClass('A'); | ||
$rcB = new ReflectionClass('B'); | ||
$rcC = new ReflectionClass('C'); | ||
$rcD = new ReflectionClass('D'); | ||
|
||
try | ||
{ | ||
$rcA->newInstanceFromData(['a' => 'bad', 'b' => 123], ['foo', 1337]); | ||
} | ||
catch(Throwable $e) | ||
{ | ||
echo "Exception: " . $e->getMessage() . "\n"; | ||
} | ||
|
||
var_dump($rcA->newInstanceFromData(['a' => 123, 'b' => 'good'], ['foo', 1337])); | ||
|
||
var_dump($rcB->newInstanceFromData(['a' => 123, 'b' => 'good'])); | ||
|
||
var_dump($rcC->newInstanceFromData(['a' => 123, 'b' => 'good'])); | ||
|
||
var_dump($rcC->newInstanceFromData([])); | ||
|
||
var_dump($rcD->newInstanceFromData(['a' => 123, 'b' => 'good'])); | ||
|
||
?> | ||
--EXPECTF-- | ||
Exception: Cannot assign string to property A::$a of type int | ||
In constructor of class A | ||
object(A)#5 (2) { | ||
["a"]=> | ||
int(123) | ||
["b"]=> | ||
string(4) "good" | ||
} | ||
object(B)#5 (2) { | ||
["a"]=> | ||
int(123) | ||
["b"]=> | ||
string(4) "good" | ||
} | ||
|
||
Deprecated: Creation of dynamic property C::$a is deprecated in %s on line %d | ||
|
||
Deprecated: Creation of dynamic property C::$b is deprecated in %s on line %d | ||
object(C)#5 (2) { | ||
["a"]=> | ||
int(123) | ||
["b"]=> | ||
string(4) "good" | ||
} | ||
object(C)#5 (0) { | ||
} | ||
object(D)#5 (2) { | ||
["a"]=> | ||
int(123) | ||
["b"]=> | ||
string(4) "good" | ||
} |
30 changes: 30 additions & 0 deletions
30
ext/reflection/tests/ReflectionClass_newInstanceFromData_002.phpt
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,30 @@ | ||
--TEST-- | ||
ReflectionClass::newInstanceFromData - internal class | ||
--FILE-- | ||
<?php | ||
|
||
$rcDateTime = new ReflectionClass('DateTime'); | ||
$rcPDOStatement = new ReflectionClass('PDOStatement'); | ||
|
||
try | ||
{ | ||
$rcDateTime->newInstanceFromData([], ['now', new DateTimeZone('UTC')]); | ||
} | ||
catch(Throwable $e) | ||
{ | ||
echo "Exception: " . $e->getMessage() . "\n"; | ||
} | ||
|
||
try | ||
{ | ||
$rcPDOStatement->newInstanceFromData(['a' => 123]); | ||
} | ||
catch(Throwable $e) | ||
{ | ||
echo "Exception: " . $e->getMessage() . "\n"; | ||
} | ||
|
||
?> | ||
--EXPECTF-- | ||
Exception: Class DateTime is an internal class that cannot be instantiated from data | ||
Exception: Class PDOStatement is an internal class that cannot be instantiated from data |
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
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.
This may throw for internal classes.
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.
It's the same code used by
newInstance
andnewInstanceArgs
which I'm thinking of refactoring into a shared function to cut the duplication, so would make sense to handle instantiating internal classes there but could be considered a breaking change?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 sure whether that's really a BC break? It would be throwing an exception either way?
We had a discussion a while ago with @DanielEScherzer and I think @nielsdos that it would probably be better to do something to fix the semantics regarding failing
get_constructor
, as various other places assume that you can even just check theconstructor
pointer on the CE.Probably the best solution is that if the
constructor
field on the CE isNULL
it means that it is not instantiable, and to set that field for classes that do not have a constructor to thezend_pass_function
(which is what get's "called" anyway in those cases) but the downside of this approach is that you stop getting useful errors telling you how to get such an object (e.g.curl_init()
forCurlHandle
objects)Uh oh!
There was an error while loading. Please reload this page.
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.
Oh I see what you mean - because this method sets properties before calling constructors that can have weird (and potentially vulnerable) behaviour.
I've added a check for
ce->type == ZEND_INTERNAL_CLASS
similar tonewInstanceWithoutConstructor()
to prevent usingnewInstanceFromData()
for internal classes entirely.