Support serialization of private properties

mxr576 → closed merge request !1328

MR#2228 resurrected, re-based and currently contains the original Closure based serialization approach.

I have also figured out why the Reflection based serialization approach broke a test. (Spoiler: never unset object properties in runtime...)

However, since Drupal core already uses the Closure based approach in ReverseContainer (since this commit), I am uncertain if I should switch back to the Reflection based serialization approach here or not. Feedback are welcomed!

okay, so I could not resist, I ran a new PHPBench test suite and it confirmed that Closure should be the winner.

$ ./vendor/bin/phpbench run core/lib/Drupal/Core/DependencyInjection/DependencySerializationBench.php --tag=closure --retry-thresho
ld=5 --report=aggregate --ref=reflection
PHPBench (1.2.14) running benchmarks... #standwithukraine
with configuration file: /mnt/files/local_mount/build/phpbench.json
with PHP version 8.1.12, xdebug ✔, opcache ❌
comparing [closure vs. reflection]

\Drupal\Core\DependencyInjection\DependencySerializationBench

    benchSerialization......................R2 I60 - [Mo15.018μs vs. Mo17.459μs] -13.98% (±1.43%)

Subjects: 1, Assertions: 0, Failures: 0, Errors: 0
Storing results ... OK
Run: 134b37696cb12d58b88f19f2df141c9855120ac2
+------------------------------+--------------------+-----+------+-----+----------------+------------------+----------------+
| benchmark                    | subject            | set | revs | its | mem_peak       | mode             | rstdev         |
+------------------------------+--------------------+-----+------+-----+----------------+------------------+----------------+
| DependencySerializationBench | benchSerialization |     | 1000 | 100 | 2.952mb -0.02% | 15.018μs -13.98% | ±1.43% -10.82% |
+------------------------------+--------------------+-----+------+-----+----------------+------------------+----------------+

Also leaving a patch file here for those who would like to use the latest state of MR on Drupal core 10.1.x, because the latest MR diff fails to apply due to phpstan-baseline.neon differences.

For data nerds like me, get_object_vars() vs Closure - of course, serializing private properties comes with a relatively small price

PHPBench (1.2.14) running benchmarks... #standwithukraine
with configuration file: /mnt/files/local_mount/build/phpbench.json
with PHP version 8.1.12, xdebug ✔, opcache ❌
comparing [get_object_vars vs. closure]

\Drupal\Core\DependencyInjection\DependencySerializationBench

    benchSerialization......................R1 I79 - [Mo13.995μs vs. Mo14.962μs] -6.47% (±1.55%)

Subjects: 1, Assertions: 0, Failures: 0, Errors: 0
Storing results ... OK
Run: 134b3775801b90c3dfeb141a25a792c5f1705cb2
+------------------------------+--------------------+-----+------+-----+----------------+-----------------+----------------+
| benchmark                    | subject            | set | revs | its | mem_peak       | mode            | rstdev         |
+------------------------------+--------------------+-----+------+-----+----------------+-----------------+----------------+
| DependencySerializationBench | benchSerialization |     | 1000 | 100 | 2.951mb -0.02% | 13.995μs -6.47% | ±1.55% +53.33% |
+------------------------------+--------------------+-----+------+-----+----------------+-----------------+----------------+

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

This looks good.

FTR, opened 🐛 Do not unset class properties because it causes un-initialization Active as a follow on #46

I could be missing something here, or perhaps PHP has moved on.

But in my testing get_object_vars supports private properties.

https://3v4l.org/ApUpq

Or is this about objects extending other objects with private properties?

https://3v4l.org/d6WN9

But if so that feels counter intuitive because access to private properties on extensions isn't possible anyway?

Or is this about objects extending other objects with private properties?

Yes

But if so that feels counter intuitive because access to private properties on extensions isn't possible anyway?

extensions? sub classes? it is true but when an object hierarchy (inheritance tree) is hydrated then dehydrated these properties also needs to be dumped and restored otherwise we end up with malformed objects.

Using private properties is a rather uncommon phenomenon in Drupal core, maybe the situation is better nowadays (have not checked) but when I opened this issue it was. Parent classes does not have expose their internal state to child classes, otherwise they would be harder to refactor or more open for modification rather than extension (O from SOLID).

Also quoting @gabesullice from #3019332-57: Use final to define classes that are NOT extension points →

As a community, we shouldn't be afraid of the final and private keywords. They exist in every major OOP language for a reason and we are not above (or below) those reasons. We should use them with good, engineering discretion.

That seems reasonable to me

Using private properties is a rather uncommon phenomenon in Drupal core,

Thankfully. Some of my advocacy still survives then.

maybe the situation is better

How is stopping extension better? Leave that to Symfony.

Parent classes do not have expose their internal state to child classes

And then how can we fix their undesirable behavior?

otherwise they would be harder to refactor or more open for modification rather than extension (O from SOLID).

I never liked solid dry pies.

@Ghost of Drupal Past - an alternate view on the last twenty years is that we're amazing at finding all possible bugs due to side effects.

So this fix is a trade-off - see https://3v4l.org/MNbOi - it sacrifices being able to restore privates from the parent that does the use DependencySerializationTrait; to give us the ability to set it on the child. At the very least we need to document this on the trait and problem inline with the use of the closure.

I think this fix might introduce some quite tricky bugs - consider the case where you have two classes extending each other both with privates with the same name. I think this is what @Ghost of Drupal Past might be getting at with the limitation to only use privates on final classes. OTOH these bugs exist already in different forms - I do wonder about the impact of the behaviour change.

@Ghost of Drupal Past FWIW there is a replacement for the coding standards TWG that potentially will reply and do the work quicker - see https://www.drupal.org/project/coding_standards →

@alexpott thanks for your review, yes, that potential edge case exists, but what is your recommendation? You moved the ticket back to "needs work", but maybe the concern you raised only means that this change can be only introduced in a new major version - or with a feature flag in a minor version? - because it changes how private props are serialized?

IMO Closure works as expected for private properties in child classes, get_object_vars() was a buggy solution in that regard as well.

So this fix is a trade-off - see https://3v4l.org/MNbOi - it sacrifices being able to restore privates from the parent that does the use DependencySerializationTrait; to give us the ability to set it on the child.

I tried to wrap my head around it and the answer could be simple... but when this change could become a problem? Private properties are scoped to the class that introduced them: https://3v4l.org/2JvMh

At the very least we need to document this on the trait

I agree that this behavior can be documented on the trait and it can be also documented in a change record.

In my mind this is a closed won't fix because the trade-offs of the closure are worse than get_object_vars()... it's more often what you want. And if you use a private on a class that extends a class from somewhere else and that class uses the trait... then you can just use the trait. And it'll work. If the class you extend from has a private and uses the trait then you need to be more careful and I'd argue that you can no longer use privates if you want dependency serialization. See https://3v4l.org/3eoXc for more.

Class B
get_object_vars()
Array
(
    [a] => 1
)


Closure
Array
(
)

This is the problematic part, right? You are assuming that after dehydration, the value of the "a" property is going to be missing, am I right? But is it?

Admittedly, I could be blindsided because I really would like to get this issue fixed... Can you pinpoint what is the unexpected outcome in my updated code example: https://3v4l.org/Yfrhp?

I even checked whether the value of the private "a" property properly resolved after serialization/deserialization in different scopes (see scopeCechoA() and scopeCechoD()).

If the class you extend from has a private and uses the trait, then you need to be more careful, and I'd argue that you can no longer use privates if you want dependency serialization.

Well, that would mean you cannot write code that is open for extension and closed for modification in some conditions, can you? And it would also mean that optional configuration introduced in PHPStan Drupal is the only way to save people from some hard-to-debug, DrupalFTW moments.

How do other frameworks handle this problem? Object hydration is quite a basic task nowadays.

With HEAD / get_object_vars

With the way it is currently if you add a private property to a class that uses the serialization trait … you can add the serialization trait too and you’ll get your property serialized… so you have two work arounds… you can either change your property to protected or you can use the trait…

If the thing you are extending has a private and is using the trait you only have 1 work around - you have to change your property to protected - you cannot have a private.

With this MR / closure

If you add a private property and extend then your private property is serialized…. yay… but if the thing you extend has a private property it will not be serialized… and there is no workaround.

Conclusion

Therefore HEAD is a better place to be. Because there are workarounds. Yes you are restricted from what you can do if the base class has privates… but that is more true with the closure.

Unless we can find something that improves the situation by serializing the privates in each class in the inheritance chain I think we’re better off with HEAD.