Fluent Interface missing on ConsoleModel::setErrorLevel #153
Conversation
| $actual = $model->setErrorLevel(0); | ||
| $this->assertSame($model, $actual); | ||
| } | ||
| } |
There was a problem hiding this comment.
A new line is missing here (at the end of the file), causing the build to fail.
19c995e to
593868e
Compare
| @@ -0,0 +1,26 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Add license and copyright.
See here: https://github.com/zendframework/maintainers/blob/abab4a99f3e6d1fac4d7400232ce30ee0bc95249/template/src/ConfigProvider.php#L2-L6
| public function testImplementsModelInterface() | ||
| { | ||
| $model = new ConsoleModel(); | ||
| $this->assertInstanceOf('Zend\View\Model\ModelInterface', $model); |
There was a problem hiding this comment.
It's better to use the class constant, instead of writing the FQN as a string.
There was a problem hiding this comment.
Please also note the comments from @thexpand. Thank you in advance!
| @@ -31,10 +31,12 @@ class ConsoleModel extends ViewModel | |||
| * Set error level to return after the application ends. | |||
| * | |||
| * @param int $errorLevel | |||
| * @return $this | |||
There was a problem hiding this comment.
@froschdesign why self? I think $this is the most right here, as we return the same instance, not the new one. I think this is also what is suggested in PSR-5 draft: https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md - check the bottom of the document: points 13-15.
There was a problem hiding this comment.
@webimpress
I would like to remove it completely! (>=7.1)
There was a problem hiding this comment.
@froschdesign Removing them is a different story and this is not answer for my question, why you think self is more appropriate here ;-)
As I understand fluent interfaces makes sens in some cases, and not always they are "evil".
https://ocramius.github.io/blog/fluent-interfaces-are-evil/
There was a problem hiding this comment.
@webimpress
No, I mean remove the return type in the DocBlock.
There was a problem hiding this comment.
@froschdesign I think what was decided in ZF is to remove them (PHPDocs) only if they are not providing any additional information. And in that case, even if we add RTH : self still @return $this gives us more information about returned object (as described - in quoted before - PSR-5 draft).
There was a problem hiding this comment.
Then I will stop the discussion here. It makes no sense without any coding standard.
There was a problem hiding this comment.
|
Thanks, @porebskk. I made some minor consistency changes to the unit test, and cherry-picked this against develop for release with 2.11.0. |
Hello,
contrary to almost all other setter methods on classes that implement the ModelInterface, ConsoleModel::setErrorLevel ist not returning itself when the method is called.
I would assume that it would return $this.
Best regards