Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve type inference for stateful container implementations #210

Merged
merged 4 commits into from
Jul 20, 2023
Merged

Improve type inference for stateful container implementations #210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a87f32
Improve type inference for stateful container implementations
gsteel Jul 19, 2023
0037c8c
Reduce calls to $this->getContainer() in favour of a local variable.
gsteel Jul 19, 2023
4a2ba1a
Reduce psalm issues in tests
gsteel Jul 19, 2023
4364ebd
Revert BC breaking parameter type changes and mark helpers `@final` w…
gsteel Jul 20, 2023
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
381 changes: 47 additions & 334 deletions psalm-baseline.xml
View file
Open in desktop

Large diffs are not rendered by default.

99 changes: 58 additions & 41 deletions src/Helper/HeadLink.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,18 @@
namespace Laminas\View\Helper;

use Laminas\View\Exception;
use Laminas\View\Helper\Placeholder\Container\AbstractContainer;
use Laminas\View\Helper\Placeholder\Container\AbstractStandalone;
use stdClass;

use function array_intersect;
use function array_keys;
use function array_shift;
use function call_user_func_array;
use function count;
use function func_get_args;
use function get_object_vars;
use function implode;
use function is_array;
use function is_object;
use function is_string;
use function method_exists;
use function preg_match;
Expand All @@ -24,11 +25,8 @@

use const PHP_EOL;

// @codingStandardsIgnoreStart
/**
* Laminas_Layout_View_Helper_HeadLink
*
* @see http://www.w3.org/TR/xhtml1/dtds.html
* @extends AbstractStandalone<int, object>
*
* Creates the following virtual methods:
* @method HeadLink appendStylesheet($href, $media = 'screen', $conditionalStylesheet = '', $extras = [])
Expand All @@ -39,9 +37,9 @@
* @method HeadLink offsetSetAlternate($index, $href, $type, $title, $extras = [])
* @method HeadLink prependAlternate($href, $type, $title, $extras = [])
* @method HeadLink setAlternate($href, $type, $title, $extras = [])
* @final
*/
class HeadLink extends Placeholder\Container\AbstractStandalone
// @codingStandardsIgnoreEnd
class HeadLink extends AbstractStandalone
{
/**
* Allowed attributes
Expand Down Expand Up @@ -84,13 +82,13 @@ public function __construct()
* Allows calling $helper->headLink(), but, more importantly, chaining calls
* like ->appendStylesheet()->headLink().
*
* @param array|null $attributes
* @param array<string, mixed>|null $attributes
* @param string $placement
* @return HeadLink
*/
public function headLink(?array $attributes = null, $placement = Placeholder\Container\AbstractContainer::APPEND)
public function headLink(?array $attributes = null, $placement = AbstractContainer::APPEND)
{
return call_user_func_array([$this, '__invoke'], func_get_args());
return $this->__invoke($attributes, $placement);
}

/**
Expand All @@ -99,22 +97,22 @@ public function headLink(?array $attributes = null, $placement = Placeholder\Con
* Returns current object instance. Optionally, allows passing array of
* values to build link.
*
* @param array|null $attributes
* @param string $placement
* @return HeadLink
* @param array<string, mixed>|null $attributes
* @param string $placement
* @return $this
*/
public function __invoke(?array $attributes = null, $placement = Placeholder\Container\AbstractContainer::APPEND)
public function __invoke(?array $attributes = null, $placement = AbstractContainer::APPEND)
{
if (null !== $attributes) {
$item = $this->createData($attributes);
switch ($placement) {
case Placeholder\Container\AbstractContainer::SET:
case AbstractContainer::SET:
$this->set($item);
break;
case Placeholder\Container\AbstractContainer::PREPEND:
case AbstractContainer::PREPEND:
$this->prepend($item);
break;
case Placeholder\Container\AbstractContainer::APPEND:
case AbstractContainer::APPEND:
default:
$this->append($item);
break;
Expand Down Expand Up @@ -201,12 +199,14 @@ public function __call($method, $args)
/**
* Check if value is valid
*
* @param mixed $value
* @internal This method will become private in version 3.0
*
* @param mixed $value
* @return bool
*/
protected function isValid($value)
{
if (! $value instanceof stdClass) {
if (! is_object($value)) {
Ocramius marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

Expand All @@ -223,9 +223,9 @@ protected function isValid($value)
/**
* append()
*
* @param mixed $value
* @param object $value
* @throws Exception\InvalidArgumentException
* @return Placeholder\Container\AbstractContainer
* @return AbstractContainer
*/
public function append($value)
{
Expand All @@ -241,8 +241,8 @@ public function append($value)
/**
* offsetSet()
*
* @param string|int $index
* @param array $value
* @param int $index
* @param object $value
* @throws Exception\InvalidArgumentException
* @return void
*/
Expand All @@ -260,9 +260,9 @@ public function offsetSet($index, $value)
/**
* prepend()
*
* @param mixed $value
* @param object $value
* @throws Exception\InvalidArgumentException
* @return Placeholder\Container\AbstractContainer
* @return AbstractContainer
*/
public function prepend($value)
{
Expand All @@ -278,9 +278,9 @@ public function prepend($value)
/**
* set()
*
* @param array $value
* @param object $value
* @throws Exception\InvalidArgumentException
* @return HeadLink
* @return $this
*/
public function set($value)
{
Expand All @@ -290,12 +290,16 @@ public function set($value)
);
}

return $this->getContainer()->set($value);
$this->getContainer()->set($value);

return $this;
}

/**
* Create HTML link element from data item
*
* @internal This method will become private in version 3.0
*
* @return string
*/
public function itemToString(stdClass $item)
Expand Down Expand Up @@ -356,24 +360,27 @@ public function itemToString(stdClass $item)
*/
public function toString($indent = null)
{
$indent = null !== $indent
? $this->getWhitespace($indent)
: $this->getIndent();
$container = $this->getContainer();
$indent = null !== $indent
? $container->getWhitespace($indent)
: $container->getIndent();

$items = [];
$this->getContainer()->ksort();
foreach ($this as $item) {
$container->ksort();
foreach ($container as $item) {
$items[] = $this->itemToString($item);
}

return $indent . implode($this->escape($this->getSeparator()) . $indent, $items);
return $indent . implode($this->escape($container->getSeparator()) . $indent, $items);
}

/**
* Create data item for stack
*
* @param array $attributes
* @return stdClass
* @internal This method will become private in version 3.0
*
* @param array<string, mixed> $attributes
* @return object
*/
public function createData(array $attributes)
{
Expand All @@ -383,8 +390,10 @@ public function createData(array $attributes)
/**
* Create item for stylesheet link item
*
* @deprecated This method is unused and will be removed in version 3.0 of this component
*
* @param array $args
* @return stdClass|false Returns false if stylesheet is a duplicate
* @return object|false Returns false if stylesheet is a duplicate
Comment on lines -387 to +396
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should hold this back a bit - this change is potentially breaking

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a deprecated, undocumented and internally unused method and it still actually returns a stdClass. For HeadLink specifically, the internal type is just (object) array<string, mixed>, in other implementations, documented return types have been changed to Psalm object shapes. I can't see any practical way that BC is broken here.

Returning object here just satisfies the template for TValue.

Can you explain how this might be a BC break?

Copy link
Member

Choose a reason for hiding this comment

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

The expectation downstream is stdClass, not object, heh :D

*/
public function createDataStylesheet(array $args)
{
Expand Down Expand Up @@ -435,6 +444,8 @@ public function createDataStylesheet(array $args)
/**
* Is the linked stylesheet a duplicate?
*
* @internal This method will become private in version 3.0
*
* @param string $uri
* @return bool
*/
Expand All @@ -452,9 +463,11 @@ protected function isDuplicateStylesheet($uri)
/**
* Create item for alternate link item
*
* @deprecated This method is unused and will be removed in version 3.0 of this component
*
* @param array $args
* @throws Exception\InvalidArgumentException
* @return stdClass
* @return object
*/
public function createDataAlternate(array $args)
{
Expand Down Expand Up @@ -493,8 +506,10 @@ public function createDataAlternate(array $args)
/**
* Create item for a prev relationship (mainly used for pagination)
*
* @deprecated This method is unused and will be removed in version 3.0 of this component
*
* @param array $args
* @return stdClass
* @return object
*/
public function createDataPrev(array $args)
{
Expand All @@ -507,8 +522,10 @@ public function createDataPrev(array $args)
/**
* Create item for a prev relationship (mainly used for pagination)
*
* @deprecated This method is unused and will be removed in version 3.0 of this component
*
* @param array $args
* @return stdClass
* @return object
*/
public function createDataNext(array $args)
{
Expand Down
Loading