-
Notifications
You must be signed in to change notification settings - Fork 61
Fix Placeholder view helper return value #162
base: master
Are you sure you want to change the base?
Conversation
…m the Placeholder::getContainer() method
…endencies on PHP 7.2
src/Helper/Placeholder.php
Outdated
@@ -37,15 +36,12 @@ class Placeholder extends AbstractHelper | |||
* Placeholder helper | |||
* | |||
* @param string $name | |||
* @throws InvalidArgumentException | |||
* @return Placeholder\Container\AbstractContainer | |||
* @return Placeholder\Container\AbstractContainer|Placeholder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self
*/ | ||
public function __invoke($name = null) | ||
{ | ||
if ($name === null) { | ||
throw new InvalidArgumentException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular change is most likely a BC break. Although I don't think people actually do rely on try-catching it, it is possible that they rely on the page to be crashing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with you. Should we move this to the 3.0.0
milestone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ocramius ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thexpand
Next major version makes sense, because nobody uses a try-catch-block here.
What could we do about the failing Travis build? Should I revert the changes, made to the EDIT: |
This repository has been closed and moved to laminas/laminas-view; a new issue has been opened at laminas/laminas-view#7. |
This repository has been moved to laminas/laminas-view. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:
|
This PR aims to fix the bug, outlined in the following issue: #16
The changes will allow
$this
as a return value of the view helper, when no name is passed as an argument, thus providing access to the methods inside the view helper. For this reason, more tests have been added toZendTest\View\Helper\PlaceholderTest
.