Skip to content
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.

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

Fix setting readonly properties in different scopes #10059

Conversation

chapterjason
Copy link
Contributor

@chapterjason chapterjason commented Sep 22, 2022

Looks like #10049 can be fixed like this, it simply uses the described method from the RFC by changing the scope to the declaring class.

It will only be executed if the isReadOnly-method is present, this will also check if PHP is 8.1 or higher, I am not sure if there is any performance impact on this.

As I described in the tests/Doctrine/Tests/ORM/Functional/Ticket/GH10049Mocks.php file, it is required to have them outside, otherwise, the file will be fully parsed by PHP 7.4 and will throw an error because the keyword readonly is new and not backwards compatible.

Fixes: #10049

@chapterjason chapterjason force-pushed the b/10049/inheritance-readonly-hydration branch from f67c840 to 0693ae3 Compare September 22, 2022 20:48
@chapterjason chapterjason marked this pull request as ready for review September 22, 2022 21:37
@derrabus
Copy link
Member

It will only be executed if the isReadOnly-method is present, this will also check if PHP is 8.1 or higher, I am not sure if there is any performance impact on this.

There is: if (PHP_VERSION_ID >= 80100) can be optimized by the compiler. Apart from that, this check is easier to spot when we drop support for older PHP versions eventually. This is why I would prefer PHP version checks over feature detection in this case.

@chapterjason
Copy link
Contributor Author

Good to know, thanks, I will adjust that.

michnovka
michnovka previously approved these changes Oct 18, 2022
Copy link
Contributor

@michnovka michnovka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok to me

@chapterjason chapterjason force-pushed the b/10049/inheritance-readonly-hydration branch from d1844a2 to fc9ef70 Compare December 25, 2022 06:35
@MatTheCat
Copy link
Contributor

MatTheCat commented May 1, 2023

This PR handles embeddables but other properties also are impacted by #10049 😕

Copy link
Contributor

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Nov 11, 2024
Copy link
Contributor

This pull request was closed due to inactivity.

@github-actions github-actions bot closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP readonly properties and inheritance cause error on hydration
4 participants