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

[WinterSilence]

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
}

[kenjis]

@WaldemarStanislawski I made a PR #4093

[MGatner]

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]

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

[WinterSilence]

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

[samsonasik]

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

[sfadschm]

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]

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]

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

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

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

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]

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

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