Support serialization of private properties

First raw patch by using Reflection, let's see how it performs and if anything fails with it.

Let's combine Closures with Reflections.

Did some digging, so #4 was an actual correct fix for the problem and it discovered a missing property issue in ModulesListForm. That caused the 8 failing tests.

1580662686|Drupal\system\Form\ModulesListForm|172.25.0.6|http://webserver/batch?id=5&op=start|http://webserver/admin/modules|1||accessManager property does not exist

Monitor and measure performance changes:

If I am reading logs correctly, the code became even faster than it was before
https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/12484/ 1h 5 mins
https://dispatcher.drupalci.org/job/drupal_patches/30560/ 1h 2 mins

Open for suggestions about how to fix this issue in a different way and how to profile it in a better way. Based on the CI, this fix even improves the speed a little bit. :O

rerolling patches...

Other than the result in #6 and also comparing the latest test execution times with and without the patch, what else we can do to profile this code. I would appreciate some help with this.

(The state of #6 is being used in production since the patch was attached.)

Just by judging based on the test execution time, there is no performance degradation, both took 57 minutes

https://dispatcher.drupalci.org/job/drupal_patches/101597/
https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/44718/

Having this fixed possibly would be better for everyone than warning for a limitations of the current solution.

https://github.com/mglaman/phpstan-drupal/issues/254

./vendor/bin/phpbench run core/lib/Drupal/Core/DependencyInjection/DependencySerializationBench.php --report=aggregate --ref=original
PHPBench (1.2.3) running benchmarks...
with PHP version 7.4.26, xdebug ❌, opcache ❌
comparing [actual vs. original]

\Drupal\Core\DependencyInjection\DependencySerializationBench

    benchSerialization......................R1 I4 - [Mo5.891μs vs. Mo5.116μs] +15.15% (±1.26%)

Subjects: 1, Assertions: 0, Failures: 0, Errors: 0
+------------------------------+--------------------+-----+------+-----+----------------+-----------------+----------------+
| benchmark                    | subject            | set | revs | its | mem_peak       | mode            | rstdev         |
+------------------------------+--------------------+-----+------+-----+----------------+-----------------+----------------+
| DependencySerializationBench | benchSerialization |     | 1000 | 10  | 3.106mb +0.10% | 5.891μs +15.15% | ±1.26% -59.62% |
+------------------------------+--------------------+-----+------+-----+----------------+-----------------+----------------+

A more realistic result by excluding the setup part from the benchmark.

    benchSerialization......................I9 - [Mo4.449μs vs. Mo4.497μs] -1.08% (±0.94%)

Subjects: 1, Assertions: 0, Failures: 0, Errors: 0
+------------------------------+--------------------+-----+------+-----+---------------+----------------+----------------+
| benchmark                    | subject            | set | revs | its | mem_peak      | mode           | rstdev         |
+------------------------------+--------------------+-----+------+-----+---------------+----------------+----------------+
| DependencySerializationBench | benchSerialization |     | 1000 | 10  | 1.803mb 0.00% | 4.449μs -1.08% | ±0.94% -11.35% |
+------------------------------+--------------------+-----+------+-----+---------------+----------------+----------------+

Indicates that reflection should perform better than closures, so switched to that.

https://github.com/mark-gerarts/php-private-access-bench

Diff between the previous solution and the current one:

comparing [actual vs. original]

\Drupal\Core\DependencyInjection\DependencySerializationBench

    benchSerialization......................R1 I3 - [Mo8.669μs vs. Mo7.330μs] +18.26% (±1.71%)

Subjects: 1, Assertions: 0, Failures: 0, Errors: 0
+------------------------------+--------------------+-----+------+-----+---------------+-----------------+---------------+
| benchmark                    | subject            | set | revs | its | mem_peak      | mode            | rstdev        |
+------------------------------+--------------------+-----+------+-----+---------------+-----------------+---------------+
| DependencySerializationBench | benchSerialization |     | 1000 | 100 | 1.803mb 0.00% | 8.669μs +18.26% | ±1.71% -3.18% |
+------------------------------+--------------------+-----+------+-----+---------------+-----------------+---------------+

> d2158784 - Use reflection instead

Meh, this change break this MR, needs rework.

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% |
+------------------------------+--------------------+-----+------+-----+----------------+-----------------+----------------+

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

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 dehydrated then hydrated 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 do 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.

When the usage of private properties would become more common in Drupal core and contrib, this problem would be much of a pain than as of today.

@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.

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.