You need a bit more research on the topic, e.g. it seems the properties on interfaces are only allowed when public

https://3v4l.org/TBaXF

and you need tests for the new cases

Hi @staabm,

Thanks for leaving your review 🤠.

You need a bit more research on the topic, e.g. it seems the properties on interfaces are only allowed when public

Right, I knew about it but missed it here.

So in case of protected/private properties:

a) We should just report Interfaces may not include properties.
b) We should report Interfaces may include only public properties. and leave the property.inInterface
c) We should create a separate class with message Interfaces may include only public properties. and id property.nonPublicInInterface
d) anything else

I bet the option c), but I prefer to ask as it's my first time contrib to phpstan :).

At the same time, I guess we should leave this implementation as-is (so property.inInterface is never reported on PHP 8.4 and above) and just create a new rule as mentioned in c).

Please wait for my review before further steps 😊

@ondrejmirtes I've implemented all suggestions, and I'm ready for the next ones (if needed 😅).

I was focused on interfaces and fixing the issue with 2.0.x not working with PHP 8.4 while creating this PR. Of course, if you decide this PR should cover property hook as a whole, just let me know, and I'll adjust the PR.

However, from my POV, the first and second bullets are conflicted. If we want to add handling abstract classes with the logic inside the NonEmptyPropertyHookRule, I'd recommend leaving it as a separate class (as PropertyInInterface doesn't sound good in case we add abstract classes there).

In my humble opinion, we should stay with split classes as they follow SRP and new features seem to not fit the Property in Interface as we are in the property hook context.

So my suggestion is to leave the separation and cover abstract classes as a separate PR, as in such a case this PR could be merged as a bugfix for 2.0, which would be nice (at least from my perspective) once I want to use 8.4 as soon as possible.

Of course, I'm waiting for your decision 🖖🏻.

I don't want this PR to completely implement property hooks, that's a much bigger project. I just want it to check what I said I want it to check. So to report a wrong declaration that's detectable just by looking at it.

I don't even want this rule to check missing implementation of an abstract hook from a parent. That's a job for a different rule.

Here's a complete list of things that should be reported by this (single) rule class after this PR:

1. On PHP before 8.4: interfaces can't have properties
2. On PHP before 8.4: properties can't have hooks
3. (All following items are on 8.4+). Properties in interfaces must have at least one hook, and all hooks must not have a body.
4. Non-abstract properties in classes must only have hooks with bodies.
5. Abstract property can only be in an abstract class.
6. Abstract property must have at least one hook without a body.
7. Property in an interface must be public.

I'd understand if you decide not to work on this. In that case you can close this PR.

But in my eyes this is the minimal work needed for the rule to make sense if it's supposed to understand "some properties can now be in interface" which is what you want to achieve.

I'd understand if you decide not to work on this. In that case you can close this PR.

Nah, that's not the case. I'll do my best to deliver this :).

I'll use the list you provided while reimplementing it. I guess around the end of the next week.

Just last thing — where should I put the code touching abstract classes?

• On PHP before 8.4: interfaces can't have properties
• On PHP before 8.4: properties can't have hooks
• Properties in interfaces must have at least one hook, and all hooks must not have a body.
• Property in an interface must be public.

Are covered by the latest changes :). The rest will be covered later this week.

By the way, I'm bumping the previous questions 😅

Just last thing — where should I put the code touching abstract classes?

I don't see any existing rule matching the new rules about property hooks, so it seems natural to create PropertiesInClassRule, but I prefer to make sure before I dive into it 😅.

Just last thing — where should I put the code touching abstract classes?

Right now you're in a rule about ClassPropertyNode. You can get $node->getClassReflection() and ask it whether it's abstract.

But yeah, these items:

• Non-abstract properties in classes must only have hooks with bodies.
• Abstract property can only be in an abstract class.
• Abstract property must have at least one hook without a body.

Could be added to a separate rule, like PropertyInClassRule. (I dislike the plural tbh.)

Despite my approach with setting the phpVersion manually failed, and I'll change it to a regular $this->markAsSkipped() once PHP is below 8.4 a new error appeared after upgrading the ondrejmirtes/better-reflection package to the latest version.

 assert($setHookBody !== null) in /Users/jacob/Projects/jakubtobiasz/phpstan-src/vendor/ondrejmirtes/better-reflection/src/Reflection/ReflectionProperty.php:699

 Caused by
 AssertionError: assert($setHookBody !== null)

Do you have any idea how to "omit" it or what I can do with it?

P.S. The code itself is finished, so you can take a look if it looks good in general.

Do you have any idea how to "omit" it or what I can do with it?

The solution is to skip the test based on PHP_VERSION_ID, or change the expected errors based on PHP_VERSION_ID. Thanks.

The solution is to skip the test based on PHP_VERSION_ID, or change the expected errors based on PHP_VERSION_ID. Thanks.

But the test is also failing on Tests / Tests (8.4, ubuntu-latest) (pull_request) & Tests / Tests (8.4, windows-latest) (pull_request) for the same reason 🤔. So it seems it's not only a problem of a test being executed on the wrong version.

From what I see in the \PHPStan\BetterReflection\Reflection\ReflectionProperty::computeImmediateVirtual should be a check whether the property:

• is in an interface
• is in an abstract class and is marked as abstract

P.S. I let myself resolve your comments; however, if your flow assumes you are resolving them after checking if they are fixed, let me know :).
P.S2 I've double checked, and everything seems to be good on my site 🙈. PHP 7.4 tests are failing due to being not able to parse the file. 8.x are failing due to the reason above 👆🏻.
P.S.3 8.3 is throwing another magic error; no idea why, but it also doesn't seem to be related to my changes 🙈.

I'll take it from here, thanks!

Feel free to check out the commits I added. There was one bug in logic (7eaed3a), the rest is basically my preferences how to organize code.

Thank you.

Thanks for the cooperation 🙌🏼

Please see:

• Community Update: All Aboard the Property Hooks Train! phpstan#12337
• Missing property hooks checks phpstan#12336