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
Open

Fix Placeholder view helper return value #162

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
991e537
Allow returning the Placeholder helper itself when no name is specified
thexpand Aug 15, 2018
c4c9488
Add test to check whether the placeholder retrieves itself when no na…
thexpand Aug 15, 2018
01e0927
Add test for the Placeholder::containerExists() method
thexpand Aug 15, 2018
d00694a
Add test to check if the created container is the same as the one fro…
thexpand Aug 15, 2018
c105734
Bump the minimum version for zend-mvc to 2.7.13 to satisfy lowest dep…
thexpand Aug 15, 2018
087b034
Switch to a range between 2.7.13 and 2.8 for zend-mvc dependency
thexpand Aug 15, 2018
4ecca91
Use the caret version range to define the minimum version for zend-mv…
thexpand Aug 15, 2018
c683c29
Use self instead of class name for the return type of Placeholder::__…
thexpand Aug 15, 2018
4be6723
Change the zend-mvc version constraint back to the original one
thexpand Aug 15, 2018
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: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"zendframework/zend-json": "^2.6.1",
"zendframework/zend-log": "^2.7",
"zendframework/zend-modulemanager": "^2.7.1",
"zendframework/zend-mvc": "^2.7 || ^3.0",
"zendframework/zend-mvc": "^2.7.13 || ^3.0",
"zendframework/zend-navigation": "^2.5",
"zendframework/zend-paginator": "^2.5",
"zendframework/zend-permissions-acl": "^2.6",
Expand Down
8 changes: 2 additions & 6 deletions src/Helper/Placeholder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

namespace Zend\View\Helper;

use Zend\View\Exception\InvalidArgumentException;
use Zend\View\Helper\Placeholder\Container;

/**
Expand Down Expand Up @@ -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.

'Placeholder: missing argument. $name is required by placeholder($name)'
);
return $this;
}

$name = (string) $name;
Expand Down
31 changes: 31 additions & 0 deletions test/Helper/PlaceholderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ public function testSetView()
$this->assertSame($view, $this->placeholder->getView());
}

/**
* @return void
*/
public function testContainerExists()
{
$this->placeholder->__invoke('foo');
$containerExists = $this->placeholder->__invoke()->containerExists('foo');

$this->assertTrue($containerExists);
}

/**
* @return void
*/
Expand All @@ -68,6 +79,15 @@ public function testPlaceholderRetrievesContainer()
$this->assertInstanceOf(AbstractContainer::class, $container);
}

/**
* @return void
*/
public function testPlaceholderRetrievesItself()
{
$container = $this->placeholder->__invoke();
$this->assertSame($container, $this->placeholder);
}

/**
* @return void
*/
Expand All @@ -77,4 +97,15 @@ public function testPlaceholderRetrievesSameContainerOnSubsequentCalls()
$container2 = $this->placeholder->__invoke('foo');
$this->assertSame($container1, $container2);
}

/**
* @return void
*/
public function testGetContainerRetrievesTheCorrectContainer()
{
$container1 = $this->placeholder->__invoke('foo');
$container2 = $this->placeholder->__invoke()->getContainer('foo');

$this->assertSame($container1, $container2);
}
}