10000 [PropertyInfo] PropertyInfo is not compatible with PHP 8.4 property hooks · Issue #58556 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[PropertyInfo] PropertyInfo is not compatible with PHP 8.4 property hooks #58556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
vudaltsov opened this issue Oct 13, 2024 · 7 comments
Closed

[PropertyInfo] PropertyInfo is not compatible with PHP 8.4 property hooks #58556

vudaltsov opened this issue Oct 13, 2024 · 7 comments
Labels
Bug PropertyInfo Status: Needs Review

Comments

@vudaltsov
Copy link
Contributor

Symfony version(s) affected

7.1.4

Description

In PHP 8.4 we'll have virtual properties and asymmetric visibility.

PropertyInfo considers the following properties writable, although they are not:

final class Foo
{
    public private(set) bool $public_private_set;
    public protected(set) bool $public_protected_set;
    public bool $virtual_no_set_hook { get => true; }
}

A side note. Many Symfony components optimistically state that they are compatible with all future PHP versions (for instance, the PropertyInfo component has "require": { "php": ">=8.2" } in its composer.json). However, this case shows that they are not and cannot be. I've discussed, whether it is a BC break on PHP's side or it isn't, see https://externals.io/message/125772. Symfony should probably reconsider it's PHP constraints.

How to reproduce

https://github.com/vudaltsov/symfony-property-access-php84

Possible Solution

In PHP 8.4 a property is writable if

   $reflectionProperty->isPublic()
&& !$reflectionProperty->isReadonly()
&& !$reflectionProperty->isPrivateSet()
&& !$reflectionProperty->isProtectedSet()
&& (!$reflectionProperty->isVirtual() || $reflectionProperty->hasHook(PropertyHookType::Set))

See https://externals.io/message/125740.

Additional Context

No response
@vudaltsov vudaltsov added the Bug label Oct 13, 2024
@vudaltsov
Copy link
Contributor Author

Also related to #54180.

@derrabus
Copy link
Member

Thank you for testing early with new versions of PHP. This way, we can make sure, Symfony is ready when PHP 8.4 is released.

Symfony should probably reconsider it's PHP constraints.

No.

In PHP 8.4 a property is writable if

   $reflectionProperty->isPublic()

&& !$reflectionProperty->isReadonly()

&& !$reflectionProperty->isPrivateSet()

&& !$reflectionProperty->isProtectedSet()

&& (!$reflectionProperty->isVirtual() || $reflectionProperty->hasHook(PropertyHookType::Set))

Sounds good. PR welcome, I suppose.

@vudaltsov
Copy link
Contributor Author

Symfony should probably reconsider it's PHP constraints.

No.

The problem is that a user with PHP 8.4 can install PropertyInfo or other downstream libraries without any problem only to find out later that it does not work with the new PHP 8.4 features.
PropertyInfo 7.1.4 is forever installable in PHP 8.4, this cannot be undone. To my mind this isn't a safe approach.

Here's what comes to my mind. Let's say, PropertyInfo requires php: >=8.2 && <8.4. Then PHP announces the feature freeze period, and the Symfony community has 2 months to ensure forward compatibility with the upcoming PHP version. Once it's done, PropertyInfo 7.1.{next patch version} is released with php: >=8.2 && <8.5 constraint. Then PHP 8.4 is out and everyone's happy, because code does not break. What's wrong with this approach?

PR welcome, I suppose.

I'll try my best to find some time :)

@vudaltsov
Copy link
Contributor Author
vudaltsov commented Oct 14, 2024 •
edited
Loading

PR welcome, I suppose.

Let's wait a little bit. ReflectionProperty::isReadable()/isWritable() methods might be introduced in PHP 8.4 if approved by RMs, see https://externals.io/message/125772#125795. Theses methods will solve the issue in a much cleaner way.

Meanwhile, the failing tests might be added.

@derrabus
Copy link
Member

Symfony should probably reconsider it's PHP constraints.

No.

The problem is that […]

Sorry, if I cut you off there. I see that you're very passionate about discussing this topic. For me, this is a topic that I consider settled for more than ten years that we use these strategy on constraints. You've opened this as a bug report to discuss an incompatibility. At least I hope that this was your intention. So please let's focus on that very issue.

If you really really really want to discuss version constraints all over again, please research old discussions on that topic, ask yourself if you really have new insights for that discussion and, if that is the case, open a new issue dedicated to that topic. Don't let the discussion about your imminent problem be derailed by something that you've called a "side note".

@derrabus
Copy link
Member

Let's wait a little bit. ReflectionProperty::isReadable()/isWritable() methods might be introduced in PHP 8.4 if approved by RMs, see https://externals.io/message/125772#125795. Theses methods will solve the issue in a much cleaner way.

That would indeed be a welcomed addition. Crossing fingers. 🤞🏻 🙂

@GromNaN GromNaN changed the title [PropertyInfo] PropertyInfo is not compatible with PHP 8.4 [PropertyInfo] PropertyInfo is not compatible with PHP 8.4 property hooks Oct 18, 2024
@pan93412
Copy link
Contributor
pan93412 commented Nov 21, 2024 •
edited
Loading

PHP 8.4 has been released today. Sadly, ReflectionProperty::isReadable() and ReflectionProperty::isWritable() are not included in PHP 8.4 (ReflectionProperty). 😥

@xabbuh xabbuh mentioned this issue Nov 21, 2024
Merged
This was referenced Nov 21, 2024
Closed
Closed
Merged
nicolas-grekas added a commit that referenced this issue Nov 25, 2024
@nicolas-grekas
bug #58959 [PropertyInfo] consider write property visibility to decid…
520e31b 
…e whether a property is writable (xabbuh)

This PR was merged into the 5.4 branch.

Discussion
----------

[PropertyInfo] consider write property visibility to decide whether a property is writable

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | part of #58556
| License       | MIT

Commits
-------

49126e1 consider write property visibility to decide whether a property is writable
nicolas-grekas added a commit that referenced this issue Nov 25, 2024
@nicolas-grekas
bug #58963 [PropertyInfo] Fix write visibility for Asymmetric Visibil…
ec691c8 
…ity and Virtual Properties (xabbuh, pan93412)

This PR was merged into the 5.4 branch.

Discussion
----------

[PropertyInfo] Fix write visibility for Asymmetric Visibility and Virtual Properties

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #58556
| License       | MIT

- [ ] Rebase after #58959 and #58962 is merged.

Commits
-------

964bf1f [PropertyInfo] Fix write visibility for Asymmetric Visibility and Virtual Properties
@vudaltsov vudaltsov mentioned this issue Dec 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug PropertyInfo Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[PropertyInfo] Implement Virtual Properties and Asymmetric Visibility support pan93412/symfony
5 participants
@nicolas-grekas @derrabus @vudaltsov @carsonbot @pan93412
0