Support unserializing 2.14 ParserResult instances

[derrabus]

Fixes doctrine/DoctrineBundle#1654

[greg0ire]

🤔 isn't this going to teach our users that it's OK to reuse their query cache from one deploy to the other? What's the story behind this PR? Are you maybe experiencing the issue yourself, with this as the only solution? According to doctrine/DoctrineBundle#1654, that cache should be automatically cleared between deploy, so something is not working. This feels like treating the symptom instead of addressing the root cause.

[derrabus]

Sorry, I should've elaborated why I chose to, well, fix this issue.

First of all, I don't want to teach anybody a lesson, I want to solve a problem. Maybe DoctrineBundle should've cleared that cache, but it didn't. Maybe DoctrineBundle needs a fix in addition to this change, but for the time being, this change solves the issue, which will buy us time to find a good solution for DoctrineBundle.

The main problem we're facing here is from my PoV only loosely related to caching: We've (accidentally) changed the serialization format of a class that is designed to be serialized. This is a breaking change that we neither intended nor documented. And the error that the developer receives (in production!) is super obscure.

The issue is easily fixable on our side. And with the tests I've added, it's also easy to maintain. Plus, I believe we can drop that code when merging up to 3.0. I don't see many reasons against this change.

[greg0ire]

I would lookup this key first, as it's the happy path.

[derrabus]

That won't work, unfortunately. When __wakeup() is called (which is the case for PHP < 7.4) with a payload created with 2.14, both keys are present.

[flack]

Just tested against php8.2 and can confirm that this fixes the problem. When you downgrade again to 2.14.2, you get the following error:

Deprecated: Creation of dynamic property Doctrine\ORM\Query\ParserResult::$sqlExecutor is deprecated in vendor/symfony/cache/Marshaller/DefaultMarshaller.php on line 74

which is kind of expected I guess.

A way to get around that would be to always serialize/deserialize to the old names (i.e. keep the serialized format stable after all, and only change the property names in the class), but I get the feeling that wouldn't be a very popular change here :-)

[derrabus]

Yes, I cannot fix the problem that you get on downgrade because that would require a change in the old code. Also, I really don't think that unserializing objects produces by a later version is really something that is expected to work.

[flack]

yes, it's a pity that this was only discovered after the release (because now serialized data exists without the underscores), otherwise you could have simply done something like this:

    private const LEGACY_PROPERTY_MAPPING = [
        'sqlExecutor' => '_sqlExecutor',
        'resultSetMapping' => '_resultSetMapping',
        'parameterMappings' => '_parameterMappings',
    ];

    public function __serialize() : array
    {
        $data = [];
        foreach (self::LEGACY_PROPERTY_MAPPING as $property => $legacyProperty) {
            $data[$legacyProperty] = $this->$property;
        }

        return $data;
    }

    public function __unserialize(array $data): void
    {
        foreach (self::LEGACY_PROPERTY_MAPPING as $property => $legacyProperty) {
            $this->$property = $data[$legacyProperty];
        }
    }

but yes, that would cause breakage if you update from 2.15 to 2.15.1 (but not if from/to 2.14 and earlier)

[flack]

btw, small footgun with the current implementation: If the ParserResult class every gets an additional property (or I suppose if someone extends it and adds a property in a child class), it will be serialized but will get dropped on deserialization.

[flack]

@derrabus thinking about it some more, if you just add a __serialize function (which converts to legacy names) to the patch you currently have, wouldn't that solve all problems? 2.15.1 will be able to read both 2.14 and 2.15 data, so if it writes 2.14 data, then downgrading to 2.14 works, and upgrading from 2.15.0 is also not a problem. The only issue then would be if you downgrade from 2.15.1. to 2.15.0, which seems relatively negligible.

[derrabus]

thinking about it some more, if you just add a __serialize function (which converts to legacy names) to the patch you currently have, wouldn't that solve all problems?

No because __serialize won't be called on PHP 7.3 and below which we still support. And really, I have no good way of testing the downgrade path. Before I go down that route, I'd rather revert to the old property names.

[flack]

ah ok, I somehow thought 7.4 was the minimum. Oh well, as the important thing is that upgrading works now