-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Bug: classes overwrites parameter defaults, phpDoc's defined in interfaces #4086
Comments
@WaldemarStanislawski I made a PR #4093 |
I'm confused how
I'd like to see what the team thinks fo this, @samsonasik maybe? |
@MGatner it's not break LSP and valid code but it's bad practice |
@MGatner but "if not specified, use the config setting." is mistake |
changing interface default value parameter will break its implementation, ref https://3v4l.org/KuM1M |
But it works the other way around (like for view):
|
In the end it might come down to the fact that every possible call that would be valid for the interface method is also valid for the implemented method and will yield the same result. E.g., when implementing and interface, additional optional parameters can be added to its methods without breaking compatibility. |
@samsonasik wrong test: <?php
interface Foo
{
public function render(bool $x = false);
}
class X implements Foo
{
public function render(bool $x = null)
{
var_dump($x);
}
}
(new X())->render(); |
@WinterSilence Thats what I meant (check the link). |
It's not true, it's not same to Also, PHP sometimes break self rules: interface Reflector {
public static function export() : string;
public function __toString() : string;
}
class ReflectionClass implements Reflector {
public static function export($argument, bool $return = FALSE) : string; // break LSP
public function __toString(): string;
} BUT, I'M DON'T TALK ABOUT CURRENT IMPLEMENTATION, NEED TEST ALL. I'M REPEAT AGAIN: IT'S NOT PHP ERROR, BUT IT'S BAD PRACTICE. |
Regardless of LSP changing the interface is a breaking change because it breaks the contract with all existing implementations. To expand on that... Currently on the So what we have amounts to a documentation versus code discrepancy. My hunch is that we leave the interface alone, remove any docs references to |
@MGatner I just explained the non-obvious features of the PHP implementation.
Yes! But |
@WinterSilence if you notice other discrepancies then please open a new issue for each. It is beyond the scope of the current maintainers to scour every interface for the PHPDocs to ensure they match. PHPStan should have caught most of these, this one was an exception because the docblock type hint was correct, the text just listed a third non-existent option. |
I agree with @MGatner. Multiple implementations of the class might not be LSP compatible, but this should not make us introduce a BC change to the interface. |
Just to be clear, that should remain for |
it's bad practice, if you want pass null need set |
right, sorry for the confusion, was just updating my commenting |
we cannot use |
You all are driving me crazy so I made #4102 - please move the discussion there and offer reviews & critiques. |
Sorry 😅 |
@MGatner |
@WinterSilence Please discuss the Pull Request on the actual PR. |
@WinterSilence |
Agreed on this. But our coding standards were designed to return |
Please fix difference(phpDoc, parameter defaults) between classes and their interfaces.
For example:
in
system/View/RendererInterface.php
$saveData = false
:in
system/View/View.php
$saveData = null
:The text was updated successfully, but these errors were encountered: