Missing methods in ApplicationConfig

[SenseException]

During my work on a puli/cli ticket I get the following Scrutinizer bug message:

The method beginOptionCommand does only exist in Webmozart\Console\Api\Config\CommandConfig, but not in Webmozart\Console\Api\Config\ApplicationConfig.

This may be an issue for projects switching from CommandConfig to ApplicationConfig.

It looks like the classes in the Config namespace need an interface or some template methods in the abstract Config class.

@webmozart: What would you prefer to make the classes replaceable by offering the same API?

[webmozart]

@SenseException Thanks for the report! Why should we add beginOptionCommand() to ApplicationConfig? Option commands only make sense within the scope of a regular command, no?

[SenseException]

In puli/cli the following code exists:

        $this
            ->beginCommand('bind')
                ->setDescription('Bind resources to binding types')
                ->setHandler(function () use ($puli) {
                    return new BindCommandHandler(
                        $puli->getDiscoveryManager(),
                        $puli->getPackageManager()->getPackages()
                    );
                })
                ->beginOptionCommand('add')

beginCommand clearly creates an instance of CommandConfig. But in the chain of methods, setHandler is part of the abstract Config class, that allows the return value to be ApplicationConfig too. Scrutinizer reads that return annotation of the DocBlock and believes that there can be other possible instances next to CommandConfig.

ApplicationConfig is extended by puli/cli and out of this class an instance of CommandConfig is created (beginCommand). There is a defined relationship between ApplicationConfig and CommandConfig and it seems that CommandConfig is a created subclass of ApplicationConfig. Even though both are of type Config doesn't this relationship make CommandConfig to a slightly different kind of config type, that is not differentiated by the code? Can puli rely that there will always be an instance of CommandConfig or the same API when the common type is Config class?

[webmozart]

Could this be fixed by changing @return static to @return $this? This isn't an architectural problem really, more that Scrutinizer isn't able to interpret the code correctly.

[SenseException]

For now, I'll look if this can be fixed on puli/cli side by adding @var annotation because I agree on that Scrutinizer is misinterpreting code.

[SenseException]

Currently it seems that only Config::setHandler() is affected. I wasn't able to trick Scrutinizer on puli/cli side with an annotation. I want to try out your suggestion about setting @return static or @return $this for Config::setHandler(), because it currently uses @return ApplicationConfig|CommandConfig|SubCommandConfig|OptionCommandConfig.

[SenseException]

@webmozart I tested the changes to @return static of Config::setHandler to see if Scrutinizer can use this info. I got 9 fixed issues: https://scrutinizer-ci.com/g/SenseException/cli/inspections/51f344b8-4078-4c1f-9a72-20b606c6035f

It seems to work that way. Would you agree if I change @return ApplicationConfig|CommandConfig|SubCommandConfig|OptionCommandConfig to @return static in a PR for Webmozart\Console\Api\Config\Config or should I test it again with @return $this?

[webmozart]

@SenseException If you could test also with return $this, that would be fabulous. I remember PHPStorm to have issues with @return static that it didn't have with @return $this.

[SenseException]

@webmozart The issues are back with @return $this: https://scrutinizer-ci.com/g/SenseException/cli/inspections/41928610-60ce-416f-983d-5aa62fa35a9c

I also tried it with @return self after that but it seems to be the same: https://scrutinizer-ci.com/g/SenseException/cli/inspections/e71450eb-4f61-46d4-8fd3-bdc655bb7e06

[webmozart]

Thanks for investigating. What's the current stance of the working group for the PhpDoc PSR on this topic?

[SenseException]

static and $this types are listed in the current PSR-5 docs
https://github.com/phpDocumentor/fig-standards/blob/master/proposed/phpdoc.md#appendix-a-types

$thisis described as "is the same exact instance as the current class in the given context". As far as I understand it, self isn't the return type we're looking for as it is referring to the class where it is defined.

[webmozart]

As per the PSR-5 docs, $this would be the correct value:

$this, the element to which this type applies is the same exact instance as the current class in the given context. As such this type is a stricter version of static as, in addition, the returned instance must not only be of the same class but also the same instance.

This type is often used as return value for methods implementing the Fluent Interface design pattern.

Let's use it then. Could you get in touch with the Scrutinizer guys to see if they can support this?

[SenseException]

Okay, I'm on it.

[webmozart]

Thanks :)

[SenseException]

scrutinizer-ci/scrutinizer#376

[SenseException]

I asked in the scrutinizer-ci ticket if I can be of help by implementing this feature request in some kind of PR, but currently I don't expect an answer.

[SenseException]

No answer on any channel so far. I'll halt my work on this issue for now.

[webmozart]

Thanks for trying @SenseException. I'll close this, since I don't want to change code to accommodate for what seems like a bug in Scrutinizer.

[SenseException]

I haven't considered alternatives to Scrutinizer yet. Maybe that would be another (bigger) approach.