Skip to content
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

Closed
WinterSilence opened this issue Jan 8, 2021 · 26 comments · Fixed by #4102
Closed

Bug: classes overwrites parameter defaults, phpDoc's defined in interfaces #4086

WinterSilence opened this issue Jan 8, 2021 · 26 comments · Fixed by #4102
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@WinterSilence
Copy link
Contributor

Please fix difference(phpDoc, parameter defaults) between classes and their interfaces.

For example:
in system/View/RendererInterface.php $saveData = false:

interface RendererInterface {
public function render(string $view, array $options = null, bool $saveData = false): string;
public function renderString(string $view, array $options = null, bool $saveData = false): string;
}

in system/View/View.php $saveData = null:

class View implements RendererInterface {
public function render(string $view, array $options = null, bool $saveData = null): string
public function renderString(string $view, array $options = null, bool $saveData = null): string
}
@WinterSilence WinterSilence added the bug Verified issues on the current code behavior or pull requests that will fix them label Jan 8, 2021
@kenjis
Copy link
Member

kenjis commented Jan 11, 2021

@WaldemarStanislawski I made a PR #4093

@MGatner
Copy link
Member

MGatner commented Jan 11, 2021

I'm confused how View is currently a valid implementation - I suppose the default type null is getting treated as a special case to make the type hint ?bool, or something? The PHPDocs make it clear that null is an intended option:

	 * @param boolean $saveData If true, will save data for use with any other calls,
	 *                          if false, will clean the data after displaying the view,
	 *                             if not specified, use the config setting.

I'd like to see what the team thinks fo this, @samsonasik maybe?

@WinterSilence
Copy link
Contributor Author

@MGatner it's not break LSP and valid code but it's bad practice

@WinterSilence
Copy link
Contributor Author

@MGatner but "if not specified, use the config setting." is mistake

@samsonasik
Copy link
Member

changing interface default value parameter will break its implementation, ref https://3v4l.org/KuM1M

@sfadschm
Copy link
Contributor

sfadschm commented Jan 12, 2021

But it works the other way around (like for view):
https://3v4l.org/BiJVZ

I think this might have something to do with what MGatner mentions. In PHP < 8 adding null as the default parameter value essentially makes a parameter nullable, but not truly. I think this might be the reason why the current implementation is valid for PHP 7.x, as such cases have been handled as rule exceptions in many places. However, I would still expect it to fail on PHP8 if that was the case.

@sfadschm
Copy link
Contributor

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.
"Furthermore the signatures of the methods must match, i.e. the type hints and the number of required arguments must be the same." Ref: https://www.php.net/manual/en/language.oop5.abstract.php

@WinterSilence
Copy link
Contributor Author

@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();

@sfadschm
Copy link
Contributor

@WinterSilence Thats what I meant (check the link).
The current implementation is perfectly valid, as we only extend the accepted value range of an optional argument (which happens to be the only argument in our case).

@WinterSilence
Copy link
Contributor Author

WinterSilence commented Jan 14, 2021

@sfadschm

as we only extend the accepted value range.

It's not true, it's not same to ?bool, PHP have some tricks/holes, they don`t break LSP.

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.

@MGatner
Copy link
Member

MGatner commented Jan 14, 2021

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 develop branch $saveData must be a boolean (according to the interface definition, ignoring the PHPDocs) and any class using RendererInterface can assume true or false, and not null. So the issue is not whether the change in the PR is LSP-safe but the implementations of it. Basically, https://3v4l.org/asm4Y

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 null and "the config setting", but View as is (with improved docs) as a LSP-safe extension of the interface. How does that sound to everyone?

@WinterSilence
Copy link
Contributor Author

WinterSilence commented Jan 14, 2021

@MGatner I just explained the non-obvious features of the PHP implementation.

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 null and "the config setting", but View as is (with improved docs) as a LSP-safe extension of the interface. How does that sound to everyone?

Yes! But RendererInterface it's only example, need check all implementations.

@MGatner
Copy link
Member

MGatner commented Jan 14, 2021

@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.

@sfadschm
Copy link
Contributor

sfadschm commented Jan 14, 2021

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.
of course, the phpDoc needs to be fixed (btw. if null is passed to render in the view class, the method really uses the config value, so that should likely stay there).

@MGatner
Copy link
Member

MGatner commented Jan 14, 2021

btw. if null is passed, the method really uses the config value, so that should likely stay

Just to be clear, that should remain for View but it should not be included on RendererInterface which has no concept of a third non-boolean option.

@WinterSilence
Copy link
Contributor Author

@sfadschm

if null is passed

it's bad practice, if you want pass null need set ?bool

@sfadschm
Copy link
Contributor

sfadschm commented Jan 14, 2021

btw. if null is passed, the method really uses the config value, so that should likely stay

Just to be clear, that should remain for View but it should not be included on RendererInterface which has no concept of a third non-boolean option.

right, sorry for the confusion, was just updating my commenting

@sfadschm
Copy link
Contributor

@sfadschm

if null is passed

it's bad practice, if you want pass null need set ?bool

we cannot use ?bool in the view class, because this is not compatible with the interface

@WinterSilence
Copy link
Contributor Author

@sfadschm yes captain, than you can't use

if null is passed to render

@MGatner need check same methods/implementations: if PHPStan ignore it here, then it can be ignored in other places.

@MGatner
Copy link
Member

MGatner commented Jan 14, 2021

You all are driving me crazy so I made #4102 - please move the discussion there and offer reviews & critiques.

@sfadschm
Copy link
Contributor

Sorry 😅

@WinterSilence
Copy link
Contributor Author

@MGatner boolean is wrong parameter type, need set bool in PhpDoc's

@MGatner
Copy link
Member

MGatner commented Jan 14, 2021

@WinterSilence Please discuss the Pull Request on the actual PR. boolean doc blocks are used throughout the framework because of an issue with CS Fixer. I believe @paulbalandan has plans to fix this at some point, but for now they should remain consistent.

@WinterSilence
Copy link
Contributor Author

WinterSilence commented Jan 14, 2021

@WinterSilence bool is valid PHP type. It`s valid for CS-Fixer/CodeSniffer/PSR-19 too.

@paulbalandan
Copy link
Member

@WinterSilence bool is valid PHP type. It`s valid for CS-Fixer/CodeSniffer/PSR-19 too.

Agreed on this. But our coding standards were designed to return boolean for bool phpdoc and integer for int phpdoc. There hasn't been any activity to the coding-standards repo to address this and other issues so I've planned on some point to migrate to PHP-CS-Fixer but met hiccups along the way. For consistency with the current formats, new phpdocs will still use the boolean and integer phpdocs until a fix or replacement is available.

@WinterSilence
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
6 participants