-
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
base: master
Are you sure you want to change the base?
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 -
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
.
|
||
const zend_class_entry *old_scope = EG(fake_scope); | ||
EG(fake_scope) = ce; | ||
constructor = Z_OBJ_HT_P(return_value)->get_constructor(Z_OBJ_P(return_value)); |
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
and newInstanceArgs
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 the constructor
pointer on the CE.
Probably the best solution is that if the constructor
field on the CE is NULL
it means that it is not instantiable, and to set that field for classes that do not have a constructor to the zend_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()
for CurlHandle
objects)
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 to newInstanceWithoutConstructor()
to prevent using newInstanceFromData()
for internal classes entirely.
Thanks @Girgias :) I'll add more test coverage based on how @DanielEScherzer feels about refactoring
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 |
…e same as `PDOStatement::fetchObject()`
Co-authored-by: Gina Peter Banyard <[email protected]>
…t sets arbitrary properties before calling the constructor which can have weird consequences
9f8a428
to
87572e9
Compare
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.