Added new proposed rules #3
Conversation
| if (!interface_exists($typeName) && class_exists($typeName)) { | ||
| // Check if this class implements any interface |
There was a problem hiding this comment.
I think there are valid cases where a class implements an interface, but is not itself logically related to said class.
For example, LoggerAwareInterface can be added to a concrete class, but that does not mean that primary use of the class is related to logging.
I think this rule should be configurable, and instead we should use a list of interfaces that we want to keep track of.
There was a problem hiding this comment.
and instead we should use a list of interfaces that we want to keep track of.
totally disagree. This would create maintanance hell. As with Finals, its not to be 100% right, I totally expect that some things will land in ignore or baseline and we were discussing that. It's just those are 1% (IDZD) I dont think we need to care about.
Even in your description - where is the actual issue? That LoggerAwareInterface will be listed as possible interface to use instead of concrete class? So what. If thats the only interface, that clearly means you are missing one, if there is more than one listed - you will pick the proper one and dont care about it at all.
And there is this tiny case, when given concrete class comes from third party and does not have any interface at all. And just add it to ignore or use phpstan ignore anotation and everything is clear during review.
| return [ | ||
| RuleErrorBuilder::message( | ||
| sprintf( | ||
| 'Class %s is not final. All non-abstract classes should be final.', |
There was a problem hiding this comment.
I'm not sure if we can make this assumption and expect to never create classes that allow extending, but are themselves not abstract/final.
There was a problem hiding this comment.
As we were disscussing exactly that case on daily - its much lower effort to think why given class is NOT FINAL and describe that case with ignore annotation / baseline entry than to check if everything is final when writing / reviewing and thinking then if something is mistake or not.
99% we are either missing final or interface.
There was a problem hiding this comment.
In general I am up for this change as it applies well to our codebase. What bothers me though is partners'/customers' code - I wonder if it's fair to report all non-abstract classes as not final. This is basically our way of doing things and I am not sure if it isn't to strict to enforce it outside.
There was a problem hiding this comment.
If thats the only issue then then can always filter whole rule out in their config with ignoreErrors.
3 participants
Description:
For QA:
Documentation: