Using typed properties on public/protected properties

[nicolas-grekas]

Right now, we added types to private/final/internal properties only because they're the only ones we can change without breaking BC.

For public/protected properties, adding a type breaks BC when a child class redeclares an existing properties. The reason is that redeclared properties must have the exact same type and their parent properties. Neither variance nor covariance is allowed for properties.

Since it'd still be nice to add types to them, can we draw a migration path?

The only path I see is to throw a deprecation when a child class redeclares a property.

The only reason I see for redeclaring a public/protected property is to change it's default value.
For non-static public/protected properties, it should always be possible to use the constructor if a different initial value is desired. As such, we could unconditionally throw a deprecation notice in DebugClassLoader when an untyped non-static property is found. Alternatively, we could we make this deprecation opt-in, eg reusing @final on properties, but I don't see the benefit of doing so since I anticipate that all such properties would have the annotation.

For static properties, I'm not sure I see a migration path. But where do we use public/protected static props in core? I can think of command names/descriptions. Those can already be replaces by attributes. So maybe the path forward is to turn them all into attributes? We'd need to investigate the codebase to have a clearer picture on that.

[derrabus]

As such, we could unconditionally throw a deprecation notice in DebugClassLoader when an untyped non-static property is found.

This would cause a lot of confusion especially if we do this on classes that don't extend Symfony classes.

But where do we use public/protected static props in core?

symfony/src/Symfony/Component/HttpFoundation/Response.php

Line 147 in e3e5946

public static $statusTexts = [

There's a handfull of places where we have public static properties. Those are grep-able. 🙂

For most cases I've found, I'd say that we expect those properties to be set from outside, but not to be redeclared in a child class. For the HttpFoundation example above, I believe that this property should be turned into a constant.

[stof]

I agree with @derrabus here. That DebugClassLoader warning probably requires an opt-in, as the DebugClassLoader does not apply only on code extending Symfony classes.

[nicolas-grekas]

It all depends on the frequency of redeclaring a public/protected property. I don't have stats, but I would think this should be pretty uncommon. If that's the case, then I think we can still do it.

[derrabus]

I've seen proprietary codebases where overriding a property with a value was a common extension point. That might be ugly design, but I don't think that our DebugClassLoader should force that opinion on existing codebases.

[nicolas-grekas]

DebugClassLoader doesn't trigger deprecations for classes in the same vendor. That should cover that.

[nicolas-grekas]

Please have a look at #45360 for non-static properties.

[nicolas-grekas]

For static properties in the Symfony codebase, here are the ones we need a plan for:

• Symfony\Component\Validator\Constraint::$errorNames
• Symfony\Component\Console\Command\Command::$defaultName
• Symfony\Component\Console\Command\Command::$defaultDescription
• Symfony\Component\Form\Test\FormPerformanceTestCase::$supportedFeatureSetVersion
• Symfony\Component\Form\Tests\AbstractLayoutTest::$supportedFeatureSetVersion

[derrabus]

• Symfony\Component\Console\Command\Command::$defaultName
• Symfony\Component\Console\Command\Command::$defaultDescription

Shall we deprecate those properties and tell people to use the attribute instead?

[nicolas-grekas]

Yes! And we might consider using a private prop or an attribute for $errorNames.
Would you like to give it a try?

[derrabus]

I'll look into the commands.

[derrabus]

#45361