Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Fluent Interface missing on ConsoleModel::setErrorLevel #153

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Model/ConsoleModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ class ConsoleModel extends ViewModel
* Set error level to return after the application ends.
*
* @param int $errorLevel
* @return $this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use self here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webimpress
I would like to remove it completely! (>=7.1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webimpress
No, I mean remove the return type in the DocBlock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I will stop the discussion here. It makes no sense without any coding standard.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/
public function setErrorLevel($errorLevel)
{
$this->options['errorLevel'] = $errorLevel;
return $this;
}

/**
Expand Down
26 changes: 26 additions & 0 deletions test/Model/ConsoleModelTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


namespace ZendTest\View\Model;

use PHPUnit\Framework\TestCase;
use Zend\View\Model\ConsoleModel;

class ConsoleModelTest extends TestCase
{
public function testImplementsModelInterface()
{
$model = new ConsoleModel();
$this->assertInstanceOf('Zend\View\Model\ModelInterface', $model);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use the class constant, instead of writing the FQN as a string.

}

/**
* @group zend-view-152
* @see https://github.com/zendframework/zend-view/issues/152
*/
public function testSetErrorLevelIsReturningThis()
{
$model = new ConsoleModel();
$actual = $model->setErrorLevel(0);
$this->assertSame($model, $actual);
}
}