Codeigniter4: 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:
~php
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:
~php
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:
~~~php
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.

[sfadschm]

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]

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]

@sfadschm

if null is passed

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

[sfadschm]

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]

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

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

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

[sfadschm]

Sorry 😅

[WinterSilence]

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

[MGatner]

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

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

[paulbalandan]

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