PHP 8.2 compatibility fixes

[forsdahl]

Currently framework code in version 4 triggers a lot of deprecation errors, because of setting dynamic properties on some classes. This can be easily fixed by declaring these properties on the classes, and should have no impact on the code otherwise.

The reason for fixing this is to be able to fix deprecation errors with PHP 8.2 in Silverstripe modules without these errors getting buried in all the deprecation errors from framework, not to have version 4 of Silverstripe officially support PHP 8.2.

[GuySartorelli]

How did you come to this set of specific properties?
My main concern here is that if it's incomplete then we may have a long and slow trickle of PRs like this over the next two years as people upgrade their projects and want to run pho 8.2 on their CMS 4 projects during the upgrade.

I'd recommend using php8.1 in your CMS 4 projects, then upgrade to CMS 5, then upgrade to PHP 8.2.

[forsdahl]

These specific properties are the ones that cause deprecation notices on /dev/build, template parsing and basically all requests handled by framework. As I mentioned, the point of this PR is not official support for PHP 8.2, just to make it possible to fix deprecation notices in other modules with CMS 4.

[GuySartorelli]

I won't have time to look at this until Monday, but here's a couple of things that need to be confirmed before this can be merged:

1. If a subclass declared these properties with some other visibility already (e.g. protected), will they now get errors for not matching the parent class signature?
2. If someone tries to access one of these properties before its value is set, is the same error or exception thrown that gets thrown without this pr?

[forsdahl]

To answer you questions:

1. Yes, if a subclass declared these same properties with some other visibility than public, then that would cause an error. However, I do not see any use case where someone would have declared these properties in any subclass, they are quite obscure.
2. No, accessing these properties before a value is set will not cause a deprecation notice or other error.

I tested these changes both with php 8.2 and with older versions, and have seen no impact except for getting rid of the deprecation notices on php 8.2. I see these changes as very low risk, declaring these properties on the classes is good practise anyway.

[GuySartorelli]

I see these changes as very low risk

That may be the case, but these are the sort of changes we would usually not make until a major release. To introduce them in a patch release requires a much higher degree of scrutiny, which is why I'm taking this through the paces. I appreciate your patience during the process.

declaring these properties on the classes is good practise anyway

Agreed, but there are many best practices we can't introduce in CMS 4 patch releases, due to semver concerns. As stated above, we need to make sure we're being very careful with anything like this as it's a wide departure from our normal release practice.

[GuySartorelli]

Please swap all public being introduced in this PR with var - this will ensure there are no breaking changes for anyone who has declared these properties on their subclasses with different visibilities and should therefore remove the last BC concern I have.

[michalkleiner]

Please swap all public being introduced in this PR with var - this will ensure there are no breaking changes for anyone who has declared these properties on their subclasses with different visibilities and should therefore remove the last BC concern I have.

@GuySartorelli are you saying that var as a synonym to public can also act as private or protected if the same variable is later redeclared in child class with protected?

[GuySartorelli]

A property declared with var is public by default, but can be overridden with a specific visibility in subclasses (so, "yes" if I understand your question correctly).
It's as close as you can get to a dynamic property while still declaring the property, from what I can tell.

[kinglozzer]

Unfortunately it still seems to error when setting non-public visibility: https://3v4l.org/SJorQ

[kinglozzer]

Can we not use the AllowDynamicProperties annotation here?

• Before: https://3v4l.org/KVVFO
• After: https://3v4l.org/PVrJu

[GuySartorelli]

Unfortunately it still seems to error when setting non-public visibility: https://3v4l.org/SJorQ

Oh weird. Didn't do that when I tried it locally but I probably just did something wrong. Thanks for double checking that.

Can we not use the AllowDynamicProperties annotation here?

I totally forgot that's a thing. Yes absolutely let's do that instead.

[forsdahl]

Good idea, totally forgot about #[\AllowDynamicProperties]! Although I think that annotation is only supported for PHP >= 8.0, but I guess PHP 7 would just ignore it?

[michalkleiner]

As in https://3v4l.org/PVrJu, it just ignores it for PHP 7.4 as it's technically a comment.
Let's go with that.

[michalkleiner]

We probably should also create (if we don't have one already) a ticket to revisit all these places for CMS 6 and do it properly.

[kinglozzer]

We probably should also create (if we don't have one already) a ticket to revisit all these places for CMS 6 and do it properly.

This is for CMS 4, I’m pretty sure it’s already been fixed in 5,. Though we might need a follow up task to remove the annotation (if it’ll get merged up automatically or might be missed if manually merging up?)

[GuySartorelli]

The automatic merge up is on a Cron so it'll be easy to deal with this before that happens. . I've already made a mental note that once this gets merged it needs to be manually merged up (and reverted during the merge up)

[forsdahl]

Changed this pull request to use the AllowDynamicProperties annotation instead of declaring these properties as public

[GuySartorelli]

Looks good to me. @kinglozzer @michalkleiner do either of you have any remaining concerns?

[GuySartorelli]

This will be automatically tagged when GitHub Actions has finished running on the branch.
I've manually reverted it in the merge-up to 5.0