[VarExporter] Fix exporting default values involving global constants

[kylekatarnls]

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
License	MIT

Hello, we hit a case when a service works fine when injected normally but stop working when injected lazily. The reason was that the default value of a parameter was a global constant that was used without explicitly specifying \ nor importing it. It ends up being incorrectly transformer in the proxy class as \MyNamespace\GLOBAL_CONSTANT instead of \GLOBAL_CONSTANT.

We worked around that by adding the \ in this service which is anyway rather a good move to be explicit. However I believe the ReflectionCaster should support this case because:

• You might want to use a class from a third-party you don't control the code as a lazy service in Symfony and they might be legit not to use the FQCN since it's a feature provided by PHP to work.
• So you can be confident that passing an injection from non-lazy to lazy is not going to cause problem.
• It minimize surprises. We were confident that mocking this service in our tests was fine and so we didn't catch it early, it would be relatively tedious if we start to assume the proxy class can be incompatible where the normal class is and so that we would have to test all non-mocked of them in all possible situations.

[kylekatarnls]

AFAIU looking at the current state of tests https://github.com/symfony/symfony/tree/5.4 it sounds like failures here are not related to the current change.

[nicolas-grekas]

Hi thanks for the PR. I'm surprised this change is in VarDumper and not in VarExporter. Does this really fix the issue you have?
Can you please share a small reproducing app just to be sure I get what we're talking about?

[kylekatarnls]

I guess both should be consistent and support what PHP natively support. For my use case, I'm looking for what is generating proxy-classes.php.

[kylekatarnls]

Basically, I think Symfony may need to use this logic anywhere it's calling getDefaultValueConstantName because the actual behavior of PHP is to try both local namesapce and global namespace but this reflection method will always only return the local namespace.

[kylekatarnls]

@nicolas-grekas You're right. It's not only applying to getDefaultValueConstantName() it also happens when doing (string) $param of a \ReflectionParameter, this precise case we hit is there in ProxyHelper::exportDefault:
https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/VarExporter/ProxyHelper.php#L338

When having in the constructor of a service:

__construct(
  #[Autowire(lazy: true)]
  private Foo $foo,
)

And Foo definition being:

namespace Bar;

class Foo
{
  public function byPi(float $number, float $pi = M_PI): float
  {
    return $number * $pi;
  }
}

Then casting to string the ReflectionParameter for float $pi = M_PI will give:

Parameter #1 [ <optional> int $pi = Bar\M_PI]

And then using this string, Symfony will assume Bar\M_PI is the correct and only way to export this default value.

Is it enough so you see what issue we face or still want me to provide a mini-repo?

I will try to make a PR to handle consistently both getDefaultValueConstantName() and (string) casts in various places I can find where it's used, but I'm open to any suggestion.

[kylekatarnls]

Updated the PR to handle only the case of ProxyHelper in VarExporter (6.4 fix)

A next (but independent) step could be #56824 so global constants can be resolved without the \ the same way in the other places.

[nicolas-grekas]

I have updated the implementation to handle more cases (eg. when consts are nested in an array)

[nicolas-grekas]

Thank you @kylekatarnls.

[kylekatarnls]

Thank to you for the quick review and improvements here 🙏