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

Fix Placeholder view helper return value #162

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

thexpand
Copy link
Contributor

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 to ZendTest\View\Helper\PlaceholderTest.

@@ -37,15 +36,12 @@ class Placeholder extends AbstractHelper
* Placeholder helper
*
* @param string $name
* @throws InvalidArgumentException
* @return Placeholder\Container\AbstractContainer
* @return Placeholder\Container\AbstractContainer|Placeholder
Copy link
Member

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(
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ocramius ping

Copy link
Member

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.

@thexpand
Copy link
Contributor Author

thexpand commented Aug 15, 2018

What could we do about the failing Travis build?
If we use zend-mvc ^2.7 it's failing on PHP 7.2 with DEPS=lowest.
If we use zend-mvc ^2.7.13 it's failing on lower PHP versions, because of the minimum stability.

Should I revert the changes, made to the composer.json?

EDIT:
I have reverted my changes. Now the composer.json is the same it was in the beginning. I saw that other PRs are also having this issue.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-view; a new issue has been opened at laminas/laminas-view#7.

@weierophinney
Copy link
Member

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:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-view to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-view.
  • In your clone of laminas/laminas-view, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants