Add test for property visibilty on final classes #202
Conversation
| <<<'PHP' | ||
| <?php | ||
| final class TheClass { | ||
| private $b; |
There was a problem hiding this comment.
In this case, I'd say that the property was indeed changed: this shouldn't produce a Changes::empty()
There was a problem hiding this comment.
Why? it doesn't make sense to have a protected property in this case. nobody can access it only the current class, so this is not a bc break. since private and protected are equal in this case. Since there is no parent class.
There was a problem hiding this comment.
PropertyChanged forwards checks to futher BCBreak checkers: it doesn't really raise a BC break itself.
In the config of this project, changes to final classes are scoped within the FinalClassChanged checks:
I think what's being hit is this other checker:
BackwardCompatibilityCheck/bin/roave-backward-compatibility-check.php
Lines 83 to 90 in 3dd7783
As you can see, not scoped within final classes context: we probably want such a check to only apply to open classes, or to public properties of final classes, so the rule has to be nested a bit deeper.
| abstract class baseClass {} | ||
|
|
||
| final class TheClass extends baseClass{ | ||
| private $b; |
There was a problem hiding this comment.
Similar here, property $b was indeed changed, but not on baseClass
There was a problem hiding this comment.
Because $b is not used in baseClass it could not be detected as a change.
There was a problem hiding this comment.
Similar to discussion at #202 (comment): I think the bug is not in the sources themselves, but rather in the configuration.
Also see Roave\BackwardCompatibility\DetectChanges\BCBreak\ClassBased\PropertyRemoved: that's the one raising this specific warning.
|
@jaapio what's the original error message that started this investigation? Maybe we can go in more detail in there, or maybe it's more of a mis-configuration issue (for |
|
|
||
| self::assertSame($propertyName, $to->getName()); | ||
|
|
||
| return Changes::fromList(Change::added($propertyName, true)); |
There was a problem hiding this comment.
The property wasn't added, since it is present in both $from and $to
|
The build which was failing can be found here: The invalid error is in my opinion here: Protected properties in a final class do not make sense. Like implemented in this issue: #31 It could be that the test cases are not completely valid, but they do illustrate the issue. Please let me know if it required another test. I can imagine that you are filtering the results when the error class is final? |
I think our best bet for now is to change the tests you wrote to target |
…ies-of-final-classes-are-not-bc-breaks Fix #202 - `PropertyRemoved` should only operate on `public` properties for `final` classes
|
Thanks for addressing this! |
This is a test case to show the issue we are facing with phpDocumentor. Both cases are failing now, but I would expect them to pass.