-
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
Add method to ReflectionClass to instantiate with the same semantics as PDOStatement::fetchObject()
#19401
Conversation
d9749d0
to
fb97b37
Compare
Hey @DanielEScherzer happy friday :)
Not sure if this one needs an RFC - I'll take your guidance on that!
But let me know your thoughts on -
- The implementation of this method, ReflectionClass::newInstanceArgs(), and ReflectionClass::newInstance() are all very similar so for the sake of not having duplicate code and therefore duplicate tests I want to refactor the "call the constructor with the given args" code into a common function.
and can you confirm that https://github.com/php/doc-base is the right repo to clone for documenting this new method?
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 don't really like the semantics exposed by PDO, and didn't know that mysqli also exposed them, as they are very dubious IMHO, as creating an object without running the constructor can lead to it being in an unexpected state.
In any case you are going to need way more tests to cover readonly
, asymmetric visibility, property hooks, and internal classes that prevent instantiation via new
.
Thanks @Girgias :) I'll add more test coverage based on how @DanielEScherzer feels about refactoring newInstance
and newInstanceArgs
because they're currently untested for those newer constructs.
I don't really like the semantics exposed by PDO, and didn't know that mysqli also exposed them, as they are very dubious IMHO, as creating an object without running the constructor can lead to it being in an unexpected state.
Yeah it's definitely unexpected to set properties before calling the constructor (do you think this needs an RFC then?) but I've found it to be powerful for ORM and DTO use-cases, for example if I've got a JSON column in my DB then with the PDO and MySQLi functions I can do something like this:
<?php final class MyClass { private string $json; public array $data; public function __construct() { $this->data = json_decode( $this->json, true ); } } $result = $pdo->query("SELECT `json` FROM `my_table`"); $record = $result->fetchObject(MyClass::class); // ... do stuff with $record->data['whatever']
For a DTO use-case I can put validation logic into my constructor like so:
<?php final class MyPayload { public int $id; public string $name; public function __construct() { if($this->id < 1) throw new InvalidArgumentException('Invalid ID'); if(!trim($this->name)) throw new InvalidArgumentException('Name cannot be blank'); } } $schema = new ReflectionClass('MyPayload'); $dto = $schema->newInstanceFromData($_POST); // ... do work on DTO knowing it's got fully valid data
9f8a428
to
87572e9
Compare
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.
please also add tests for
- constructor property promotion
- constructor property promotion with read-only properties (should trigger errors about reassigning)
- normal read-only properties getting set via the data
- normal read-only properties getting set via the data and in the constructor (should trigger errors)
- hooked properties - with a
set
hook - hooked properties - virtual properties
- using the wrong type of value in the
$data
values (e.g trying to assign a string to an object property)
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.
done - i've added a whole bunch of scenarios in ReflectionClass_newInstanceFromData_001.phpt
there are a couple of unexpected-but-not-really scenarios:
- if a property is promoted from the constructor it must be specified in
$args
, and if that property is in both$data
and$args
then the one in$args
will overwrite it in the call to the constructor. - if a property has a
set
hook or is entirely virtual then the setter will run, along with any side-effects onto other properties, in the order the properties are provided in$data
.
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.
if a property is promoted from the constructor it must be specified in $args, and if that property is in both $data and $args then the one in $args will overwrite it in the call to the constructor.
Even if the property is readonly?
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.
Ah good catch - I've added a test for this 👌 If a promoted property is readonly and a value is specified in both $data
and $args
it'll throw a "Cannot modify readonly property" when the constructor is called so it can only be in one of them.
Do you think that's a userspace issue to resolve? (ie don't put readonly values in both $data
and $args
) or have newInstanceFromData
handle it automatically by moving promoted properties from $data
to $args
and throwing an exception if a promoted property value is in both?
PDO and MySQLi leave it to userspace and so newInstanceFromData
should for consistency (plus if I leave it this way then this PR is ready 🙂) but I dislike silent "errors" and prefer to put in effort to make things Just Work for users - so maybe automatically handling it is the right approach and updating PDO/MySQLi to handle promoted properties gracefully too?
Thanks @DanielEScherzer
Just so that I understand the semantics correctly, is there any difference between the proposed function and something like (class is given as an argument but would be from $this)
Correct
There are three motivations for building into PHP though:
- this behaviour exists in PDO and MySQLi and I wanted to make it accessible to others because I've found it really useful
- there's a performance boost - if I run your sample 1000 times on the same object with 3 properties it takes 3ms whereas calling
newInstanceFromData
takes 1ms. - i've started noticing frameworks like Tempest doing a version of this (ref and ref, although it doesn't call the constructor at all) and at least one other DTO library I evaluated a couple months ago but can't remember the name of...
...e same as `PDOStatement::fetchObject()`
Co-authored-by: Gina Peter Banyard <girgias@php.net>
...t sets arbitrary properties before calling the constructor which can have weird consequences
...an assertion failure
0b655e2
to
6af7ab4
Compare
...e setting any values to avoid leaking memory
Just for managing expectations, in my role as a release manager I object to including this in PHP 8.5 - it has not been discussed in an RFC (or even the mailing list?) and the semantics are convoluted enough that if we do this, we should make sure we do it right
Given that, is my role as the maintainer of ext/reflection, I'm probably not going to be able to devote too much time to this until after PHP 8.5 is released, but I do want to highlight @Girgias's point
I don't really like the semantics exposed by PDO, and didn't know that mysqli also exposed them, as they are very dubious IMHO, as creating an object without running the constructor can lead to it being in an unexpected state.
which I agree with. It seems like this is something that can be done in userland too, based on the response in #19401 (comment) that the new function is essentially
function newInstanceFromData(string $class, array $data, array $args = []) { $ref = new ReflectionClass($class); $instance = $ref->newInstanceWithoutConstructor(); foreach ($data as $propName => $val) { $propAccess = $ref->getProperty($propName); $propAccess->setValue($instance, $val); } $instance->__construct(...$args); return $instance; }
but, implementing in userland means we don't need to worry about reflection bypassing things like constructor visibility, un-instantiable internal classes, etc. since this is all valid userland code.
At the very least, please start a discussion on the internals mailing list and see if anyone objects, in which case this will definitely need an RFC.
Thanks @DanielEScherzer and good luck with the release :)
And yes I agree that from our discussions it's worth upgrading this to the mailing list or an RFC - in short:
- do we want to make this semantic "official" or leave it as-is and semi-hidden on PDO and MySQLi
- if we do make it official, should we also update PDO and MySQLi to have the same behaviour re promoted properties, readonly, hooks, etc
- what additional safety/validation/security/convenience behaviour is worth including based on existing DTO libraries
- whether to take this opportunity to roll out ReflectionClass::isInstantiable() is wrong about internal classes that prevent instantiation #17796 at the same time
Uh oh!
There was an error while loading. Please reload this page.
I've been using
PDOStatement::fetchObject()
andmysqli_result::fetch_object()
for a couple years as a very rudimentary ORM, and I've extended that "design pattern" to create class objects from arbitrary data (eg JSON or$_POST
payloads) using a custom method.That custom approach uses
ReflectionClass::newInstanceWithoutConstructor()
andReflectionProperty::setValue()
plus some additional validation and type massaging logic to emulate the behaviour of the PDO and MySQLi methods.This PR adds a
newInstanceFromData(array $data, array $args = [])
method toReflectionClass
that behaves the same as the PDO and MySQLi methods but is actually baked into PHP properly, ie: it instantiates the class and assigns the properties to the corresponding values in$data
before calling the constructor. The second parameter$args
is passed to the class constructor the same asReflectionClass::newInstanceArgs()
.The primary use-case for this would be non-database ORM models and DTOs.
Other things I want to look at:
ReflectionClass::newInstanceArgs()
, andReflectionClass::newInstance()
are all very similar so for the sake of not having duplicate code and therefore duplicate tests I want to refactor the "call the constructor with the given args" code into a common function.(削除)actually no this doesn't make sense here because users would just callPDOStatement::fetch()
withPDO::FETCH_CLASS
also supportsPDO::FETCH_PROPS_LATE
flag to call the constructor before setting properties which makes sense here too. (削除ここまで)new MyClass
and pass data to the constructor like normal.