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

[VarExporter] add Instantiator::instantiate() to create+populate objects without calling their constructor nor any other methods #28417

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@nicolas-grekas
Member

nicolas-grekas commented Sep 9, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

A blend of features also provided by https://github.com/doctrine/instantiator and https://github.com/Ocramius/GeneratedHydrator in one simple method. Because it's just a few more lines on top of the existing code infrastructure in the component :)

For example, from the docblock:

// creates an empty instance of Foo
Instantiator::instantiate(Foo::class);

// creates a Foo instance and sets one of its public, protected or private properties
Instantiator::instantiate(Foo::class, ['propertyName' => $propertyValue]);

// creates a Foo instance and sets a private property defined on its parent Bar class
Instantiator::instantiate(Foo::class, [], [
    Bar::class => ['privateBarProperty' => $propertyValue],
]);

Instances of ArrayObject, ArrayIterator and SplObjectHash can be created
by using the special "\0" property name to define their internal value:

// creates an SplObjectHash where $info1 is attached to $obj1, etc.
Instantiator::instantiate(SplObjectStorage::class, ["\0" => [$obj1, $info1, $obj2, $info2...]]);

// creates an ArrayObject populated with $inputArray
Instantiator::instantiate(ArrayObject::class, ["\0" => [$inputArray, $optionalFlag]]);

Misses some tests for now, but reuses the existing code infrastructure used to "unserialize" objects.

Show resolved Hide resolved src/Symfony/Component/VarExporter/Instantiator.php
use Symfony\Component\VarExporter\Internal\Registry;
/**
* A utility class to create objects without calling their constructor.

This comment has been minimized.

@Ocramius

Ocramius Sep 10, 2018

Contributor

Most likely a good idea to keep this @internal until proven to be stable for usage within the component

@Ocramius

Ocramius Sep 10, 2018

Contributor

Most likely a good idea to keep this @internal until proven to be stable for usage within the component

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 10, 2018

Member

The proposed method borrows from the existing infrastructure in the component: if there are any issues with Instantiator::instantiate(), it's also an issue for VarExporter::export(). It would make no sense to mark this method as internal, and not the other.

And if we mark the other internal, then the component would have zero public interfaces. It wouldn't make sense :)

@nicolas-grekas

nicolas-grekas Sep 10, 2018

Member

The proposed method borrows from the existing infrastructure in the component: if there are any issues with Instantiator::instantiate(), it's also an issue for VarExporter::export(). It would make no sense to mark this method as internal, and not the other.

And if we mark the other internal, then the component would have zero public interfaces. It wouldn't make sense :)

Show outdated Hide outdated src/Symfony/Component/VarExporter/Instantiator.php
Show outdated Hide outdated src/Symfony/Component/VarExporter/Instantiator.php
* Instances of ArrayObject, ArrayIterator and SplObjectHash can be created
* by using the special "\0" property name to define their internal value:
*
* // creates an SplObjectHash where $info1 is attached to $obj1, etc.

This comment has been minimized.

@Ocramius

Ocramius Sep 10, 2018

Contributor

Strongly suggest keeping anything coming from php core or extensions out of the scope of the component, as well as its documentation: there's an infinity of wrongness in classed that are not defined in PHP userland, and we really should rather tell users to not rely on them.

@Ocramius

Ocramius Sep 10, 2018

Contributor

Strongly suggest keeping anything coming from php core or extensions out of the scope of the component, as well as its documentation: there's an infinity of wrongness in classed that are not defined in PHP userland, and we really should rather tell users to not rely on them.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 10, 2018

Member

Same as above: this is already dealt with for VarExporter::export(), so any issue found there should be spotted and fixed. There are already tests for these btw (which doesn't mean it's yet bulletproof of course.)

@nicolas-grekas

nicolas-grekas Sep 10, 2018

Member

Same as above: this is already dealt with for VarExporter::export(), so any issue found there should be spotted and fixed. There are already tests for these btw (which doesn't mean it's yet bulletproof of course.)

Show resolved Hide resolved src/Symfony/Component/VarExporter/README.md
Show resolved Hide resolved src/Symfony/Component/VarExporter/Instantiator.php
Show resolved Hide resolved src/Symfony/Component/VarExporter/Instantiator.php
Show resolved Hide resolved src/Symfony/Component/VarExporter/Instantiator.php
Show resolved Hide resolved src/Symfony/Component/VarExporter/Instantiator.php
@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 10, 2018

Member

Thanks for the detailed description! Looking at the end-user API, I have two comments/questions:

  1. Do we really need to support an array as the argument of instantiate()? Is that common to instantiate more than one object at a time?
// Before
Instantiator::instantiate([
    Foo::class => [],
    Bar::class => ['propertyName' => $propertyValue],
]);

// After
Instantiator::instantiate(Foo::class, []);
Instantiator::instantiate(Bar::class, ['propertyName' => $propertyValue]);
  1. The use of \0 for some special objects looks like an internal detail which complicates the learning curve when exposing it publicly. Why not "hardcode" this detail internally and let the end-user not know about this?
// Before
Instantiator::instantiate([SplObjectStorage::class => ["\0" => [$obj1, $info1, $obj2, $info2]]]);
Instantiator::instantiate([ArrayObject::class => ["\0" => $inputArray]]);

// After
Instantiator::instantiate(SplObjectStorage::class, [$obj1 => $info1, $obj2 => $info2]);
Instantiator::instantiate(ArrayObject::class, $inputArray);
Member

javiereguiluz commented Sep 10, 2018

Thanks for the detailed description! Looking at the end-user API, I have two comments/questions:

  1. Do we really need to support an array as the argument of instantiate()? Is that common to instantiate more than one object at a time?
// Before
Instantiator::instantiate([
    Foo::class => [],
    Bar::class => ['propertyName' => $propertyValue],
]);

// After
Instantiator::instantiate(Foo::class, []);
Instantiator::instantiate(Bar::class, ['propertyName' => $propertyValue]);
  1. The use of \0 for some special objects looks like an internal detail which complicates the learning curve when exposing it publicly. Why not "hardcode" this detail internally and let the end-user not know about this?
// Before
Instantiator::instantiate([SplObjectStorage::class => ["\0" => [$obj1, $info1, $obj2, $info2]]]);
Instantiator::instantiate([ArrayObject::class => ["\0" => $inputArray]]);

// After
Instantiator::instantiate(SplObjectStorage::class, [$obj1 => $info1, $obj2 => $info2]);
Instantiator::instantiate(ArrayObject::class, $inputArray);
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 10, 2018

Member

@Ocramius @javiereguiluz thanks for the review+comments, I addressed some inline, let's address the remaining ones:

I added a 2nd commit to change the signature to instantiate(string $class, array $properties, array $privateProperties);. See updated examples in the PR. If you think it's better for DX, fine to me.

The use of \0 for some special objects looks like an internal detail

It's not actually: e.g. you can also add dynamic properties to ArrayObject instances, that are not stored in the internal array structure. The instantiate() method has to provide a way to set both these properties and the internal array. \0 is specifically chosen because no dynamic properties can start with the nul character, so it is safe to use.

This doesn't mean we cannot do what you propose @javiereguiluz, but we still need a way to set dynamic properties when needed.

Actually, there is a trivial way that works already: using an stdClass key in the $privateProperties: this populates public (thus dynamic) properties already.

So, let's say we want to create something like that using instantiate:

$a = new ArrayObject($input);
$a->foo = 123;

The current code allows doing so using:

Instantiator::instantiate(ArrayObject::class, ['foo' => 123, "\0" => [$input]]);

And we could make it do the same using:

Instantiator::instantiate(ArrayObject::class, [$input], ['stdClass' => ['foo' => 123]]);

Please advise.
(note that I have a preference for the 1st variant because it makes it less an edge case, so would be easier to work with in generic code - at least that's how I feel about it.)

Member

nicolas-grekas commented Sep 10, 2018

@Ocramius @javiereguiluz thanks for the review+comments, I addressed some inline, let's address the remaining ones:

I added a 2nd commit to change the signature to instantiate(string $class, array $properties, array $privateProperties);. See updated examples in the PR. If you think it's better for DX, fine to me.

The use of \0 for some special objects looks like an internal detail

It's not actually: e.g. you can also add dynamic properties to ArrayObject instances, that are not stored in the internal array structure. The instantiate() method has to provide a way to set both these properties and the internal array. \0 is specifically chosen because no dynamic properties can start with the nul character, so it is safe to use.

This doesn't mean we cannot do what you propose @javiereguiluz, but we still need a way to set dynamic properties when needed.

Actually, there is a trivial way that works already: using an stdClass key in the $privateProperties: this populates public (thus dynamic) properties already.

So, let's say we want to create something like that using instantiate:

$a = new ArrayObject($input);
$a->foo = 123;

The current code allows doing so using:

Instantiator::instantiate(ArrayObject::class, ['foo' => 123, "\0" => [$input]]);

And we could make it do the same using:

Instantiator::instantiate(ArrayObject::class, [$input], ['stdClass' => ['foo' => 123]]);

Please advise.
(note that I have a preference for the 1st variant because it makes it less an edge case, so would be easier to work with in generic code - at least that's how I feel about it.)

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 10, 2018

Member

Thanks for the detailed explanation. I hadn't thought about this use case:

$a = new ArrayObject($input);
$a->foo = 123;

I thought this utility was only for instantiating ... but that code instantiates and then initializes, so it's doing two different things :)

What if we use this signature?

public static function instantiate(
    string $classFqcn,
    array $constructorArguments = [],
    array $propertyValues = []
)

Then, this example would look as:

Instantiator::instantiate(ArrayObject::class, [$input], ['foo' => 123])

But most of the times you'd simply use something like this:

Instantiator::instantiate(ArrayObject::class, [$input])
Member

javiereguiluz commented Sep 10, 2018

Thanks for the detailed explanation. I hadn't thought about this use case:

$a = new ArrayObject($input);
$a->foo = 123;

I thought this utility was only for instantiating ... but that code instantiates and then initializes, so it's doing two different things :)

What if we use this signature?

public static function instantiate(
    string $classFqcn,
    array $constructorArguments = [],
    array $propertyValues = []
)

Then, this example would look as:

Instantiator::instantiate(ArrayObject::class, [$input], ['foo' => 123])

But most of the times you'd simply use something like this:

Instantiator::instantiate(ArrayObject::class, [$input])
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 10, 2018

Member

@javiereguiluz wouldn't make sense: the specific property of instantiators is to not call the constructor nor any other methods...

Member

nicolas-grekas commented Sep 10, 2018

@javiereguiluz wouldn't make sense: the specific property of instantiators is to not call the constructor nor any other methods...

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 10, 2018

Member

I just followed your last example: why pass $input if it's never used?

Member

javiereguiluz commented Sep 10, 2018

I just followed your last example: why pass $input if it's never used?

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 10, 2018

Member

After talking with Nico on Slack about this ... it's clear to me that I don't fully understand this 😅 So let's forget about my previous examples and proposals. Sorry!

Member

javiereguiluz commented Sep 10, 2018

After talking with Nico on Slack about this ... it's clear to me that I don't fully understand this 😅 So let's forget about my previous examples and proposals. Sorry!

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 10, 2018

Member

Good news: shaking the code always helps to spot new edge cases.
PR is ready, with test cases and fixes found meanwhile.

Member

nicolas-grekas commented Sep 10, 2018

Good news: shaking the code always helps to spot new edge cases.
PR is ready, with test cases and fixes found meanwhile.

nicolas-grekas added a commit that referenced this pull request Sep 11, 2018

bug #28437 [VarExporter] fix more edge cases (nicolas-grekas)
This PR was merged into the 4.2-dev branch.

Discussion
----------

[VarExporter] fix more edge cases

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As found while preparing #28417

Commits
-------

443bd11 [VarExporter] fix more edge cases
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 11, 2018

Member

Fixes split in PR #28437. This PR is now rebased on top of it.

Member

nicolas-grekas commented Sep 11, 2018

Fixes split in PR #28437. This PR is now rebased on top of it.

[VarExporter] add Instantiator::instantiate() to create+populate obje…
…cts without calling their constructor nor any other methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment