Skip to content

Conversation

jakubtobiasz
Copy link
Contributor

@jakubtobiasz jakubtobiasz commented Feb 2, 2025

Fulfills Hooked properties cannot be readonly from phpstan/phpstan#12336 :)

@jakubtobiasz jakubtobiasz force-pushed the hooked-properties-cannot-be-readonly branch 2 times, most recently from 912148c to 013940a Compare February 2, 2025 18:32
@ondrejmirtes
Copy link
Member

Also please do the same in the interface property rule, thank you.

@jakubtobiasz jakubtobiasz force-pushed the hooked-properties-cannot-be-readonly branch from 1d50e87 to 72ea3ac Compare February 3, 2025 07:09
@jakubtobiasz
Copy link
Contributor Author

Hello @ondrejmirtes 👋🏼,

I've covered what you asked for :).

As I'm working on hooked properties and I see your list of to be done in that matter, what do you think about splitting PropertyInClassRule into PropertyInClassRule (non-hooked properties thingies only), HookedPropertyInAbstractClassRule, HookedPropertyInReadonlyClassRule, HookedPropertyInClassRule or something similar? I'm afraid keeping all these rules in a single class might affect readability and maintainability in the future :/.

@jakubtobiasz jakubtobiasz force-pushed the hooked-properties-cannot-be-readonly branch from 72ea3ac to 628013e Compare February 3, 2025 07:20
@ondrejmirtes
Copy link
Member

I think the current way is better. It's easier to see what is and what isn't covered. Some rules apply to all properties so with your suggestion there would have to be some duplication too.

@jakubtobiasz
Copy link
Contributor Author

Ok, so do you anything more from me in this PR or I can take a next one :D.

@ondrejmirtes ondrejmirtes force-pushed the hooked-properties-cannot-be-readonly branch from 628013e to fdda36e Compare February 4, 2025 12:33
@ondrejmirtes
Copy link
Member

I moved a piece of code for better readability (fdda36e) which means the error is reported regardless of $node->isReadOnly() and no tests have failed. Can you please confirm this still works as intended?

@jakubtobiasz
Copy link
Contributor Author

Seems legit 🤔

@ondrejmirtes ondrejmirtes merged commit 166dcbe into phpstan:2.1.x Feb 5, 2025
428 of 430 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@jakubtobiasz jakubtobiasz deleted the hooked-properties-cannot-be-readonly branch February 5, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants