Add compatibility with the Symfony 4.4 VarExporter

[cmodijk]

This issue tries to solve the weird Symfony __unserialize data Symfony 4.4 is giving to the method. This seems to be a bug that Symfony solved in 5.4 in symfony/symfony@6ccb85e. Original report / investigation #10943.

[derrabus]

Is there a way we can test this? 🤔

Also, can we revert this change on 3.0.x?

[greg0ire]

Is there a way we can test this? 🤔

In #10684, you introduced txt files containing serialized data. Maybe there should be a new text file with the 3rd format in it?

[cmodijk]

Is there a way we can test this? 🤔

The txt files containing serialized data was not really working for me as Symfony directly call's the __unserialize with the wrong formated data. So i added a new test instead to check if that mapped data is handled.

Also, can we revert this change on 3.0.x?

I'm not sure. If 3.0.x is removing support for Symfony 4.4 then yes. In the end it depends on if Doctrine ORM whants to support Symfony 4.4. When Symfony 4.4 support is dropped then this can be removed.

But maybe the whole __unserialize should be dropped at some point as it still tries to reflect the old data from version 2.14.3 and when the __unserialize method is not there Symfony 4.4 also work's as expected.

[derrabus]

I'm not sure. If 3.0.x is removing support for Symfony 4.4 then yes.

Symfony 4.4 is EOL, so yes, we won't support it on the 3.0.x branch.

[derrabus]

Can we actually use VarExporter in that test? Otherwise nobody will understand where that data structure is coming from.

[cmodijk]

Can we actually use VarExporter in that test? Otherwise nobody will understand where that data structure is coming from.

If there is a way to ensure we get the Symfony 4.4 version of the VarExporter in that test then yes. But I don't know how i could achief that. For context to @derrabus this seems to be fixed in Symfony 5.4 and higher.

[derrabus]

We do run our tests against VarExporter 4.4, yes.

[cmodijk]

@derrabus I have checked out the PHPUnit check but can't firure out where Symfony 4.4 is tested?

[greg0ire]

It's tested here: https://github.com/doctrine/orm/actions/runs/6247060166/job/16959520070?pr=10948#step:4:171

[cmodijk]

I have rewriten the test to only validate it when symfony/var-exporter versie 4.4 is used and then let it export the data and then unserialize it back to the prefered data. Would love to hear feedback if this is a correct way of testing this.

[derrabus]

I have rewriten the test to only validate it when symfony/var-exporter versie 4.4 is used

Why? I understand that the problem is only reproducible with VarExporter 4, but the test should pass with any version, shouldn't it?

[cmodijk]

Why? I understand that the problem is only reproducible with VarExporter 4, but the test should pass with any version, shouldn't it?

Yes, you are right did not think about that. I removed the skip.

[derrabus]

This is a complicated way to perform an eval(), isn't it?

[cmodijk]

I'm not sure how i can solve this i followd the Symfony docs which has noted it to use it this way.

use Symfony\Component\VarExporter\VarExporter;

$exported = VarExporter::export($someVariable);
// store the $exported data in some file or cache system for later reuse
$data = file_put_contents('exported.php', '<?php return '.$exported.';');

// later, regenerate the original variable when you need it
$regeneratedVariable = require 'exported.php';

[cmodijk]

What we maybe could do is dump the file once, commit it and then use the output to validate. But then we still need a way to import the file which is the part your question is about. So maybe this is still not the way. 🤔

[derrabus]

You don't need a file. PHP has eval(). Use that.

[cmodijk]

I have pushed a update to use eval instead.

[cmodijk]

@derrabus I have updated the code with the feedback. When you have time can you check if this is what you expected?

The base branch was changed.

[derrabus]

Thank you!

[cafferata]

Thank you too!

[cmodijk]

Yes, Thank you too! ❤️