- Merge request !2228Issue #3110266: Support serialization of private properties → (Open) created by mxr576
- last update
over 1 year ago 30,368 pass - Status changed to Needs review
about 1 year ago 10:48am 30 October 2023 - 🇭🇺Hungary mxr576 Hungary
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!
- 🇭🇺Hungary mxr576 Hungary
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% | +------------------------------+--------------------+-----+------+-----+----------------+------------------+----------------+
- 🇭🇺Hungary mxr576 Hungary
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.
- 🇭🇺Hungary mxr576 Hungary
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% | +------------------------------+--------------------+-----+------+-----+----------------+-----------------+----------------+
- Status changed to Needs work
about 1 year ago 8:32pm 2 November 2023 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.
- Status changed to Needs review
about 1 year ago 7:53am 5 November 2023 - Status changed to RTBC
about 1 year ago 7:34am 6 November 2023 - last update
about 1 year ago 30,486 pass - last update
about 1 year ago Custom Commands Failed - 🇭🇺Hungary mxr576 Hungary
FTR, opened 🐛 Do not unset class properties because it causes un-initialization Active as a follow on #46
- last update
about 1 year ago 30,516 pass - last update
about 1 year ago 30,519 pass - last update
about 1 year ago 30,530 pass - Status changed to Needs review
about 1 year ago 4:19am 15 November 2023 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I could be missing something here, or perhaps PHP has moved on.
But in my testing get_object_vars supports private properties.
Or is this about objects extending other objects with private properties?
But if so that feels counter intuitive because access to private properties on extensions isn't possible anyway?
- 🇭🇺Hungary mxr576 Hungary
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).
- 🇭🇺Hungary mxr576 Hungary
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.
- Status changed to RTBC
about 1 year ago 11:40pm 23 November 2023 - last update
about 1 year ago 30,667 pass - last update
about 1 year ago 30,674 pass, 2 fail 3:04 58:24 Running- last update
about 1 year ago 30,683 pass, 2 fail - last update
about 1 year ago 30,687 pass, 2 fail - last update
about 1 year ago 30,687 pass, 2 fail - last update
about 1 year ago 30,687 pass, 2 fail - last update
about 1 year ago 30,695 pass, 2 fail - last update
about 1 year ago 30,697 pass, 2 fail - last update
about 1 year ago 30,711 pass, 2 fail - last update
about 1 year ago 30,723 pass, 2 fail - last update
about 1 year ago 30,763 pass, 2 fail - last update
about 1 year ago 30,765 pass, 2 fail - last update
about 1 year ago 25,909 pass, 1,789 fail - last update
about 1 year ago Build Successful - last update
12 months ago Build Successful - last update
12 months ago Build Successful - last update
12 months ago Build Successful - last update
12 months ago Build Successful - last update
12 months ago Build Successful - last update
12 months ago Build Successful - last update
12 months ago Build Successful - First commit to issue fork.
- 🇨🇦Canada Charlie ChX Negyesi 🍁Canada
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.
- last update
12 months ago Build Successful - Status changed to Needs work
12 months ago 9:26am 8 January 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
@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 →
- 🇭🇺Hungary mxr576 Hungary
@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.
- 🇭🇺Hungary mxr576 Hungary
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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇭🇺Hungary mxr576 Hungary
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()
andscopeCechoD()
).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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
I tried to update to Drupal 10.3.x and I got error with the latest patch so I fixed it.