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.

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

Implement AbstractTypedCollection with tests #12089

Merged
merged 5 commits into from Jan 11, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
171 changes: 171 additions & 0 deletions src/Core/Data/AbstractTypedCollection.php
@@ -0,0 +1,171 @@
<?php
/**
* 2007-2019 PrestaShop SA and Contributors
*
* NOTICE OF LICENSE
*
* This source file is subject to the Open Software License (OSL 3.0)
* that is bundled with this package in the file LICENSE.txt.
* It is also available through the world-wide-web at this URL:
* https://opensource.org/licenses/OSL-3.0
* If you did not receive a copy of the license and are unable to
* obtain it through the world-wide-web, please send an email
* to license@prestashop.com so we can send you a copy immediately.
*
* DISCLAIMER
*
* Do not edit or add to this file if you wish to upgrade PrestaShop to newer
* versions in the future. If you wish to customize PrestaShop for your
* needs please refer to http://www.prestashop.com for more information.
*
* @author PrestaShop SA <contact@prestashop.com>
* @copyright 2007-2019 PrestaShop SA and Contributors
* @license https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
* International Registered Trademark & Property of PrestaShop SA
*/

namespace PrestaShop\PrestaShop\Core\Data;

use Doctrine\Common\Collections\ArrayCollection;
use PrestaShop\PrestaShop\Core\Exception\InvalidArgumentException;

/**
* Class AbstractTypedCollection is an abstract collection class which checks
* that the inserted elements match the requested type.
*/
abstract class AbstractTypedCollection extends ArrayCollection
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldnt it be better to have TypedCollection::__construct(string $type, array $elements = [])? so we wouldnt need abstract at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right it would work, but it changes the constructor prototype
even though you could override it again in the child class and remove this argument, the advantage I see with the abstract is that it forces you to implement it, you don't have a choice
whereas you could "forget" to override the constructor

Copy link
Contributor

@sarjon sarjon Jan 11, 2019

Choose a reason for hiding this comment

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

actually what i mean is that override would become optional. you could just do something like:

$objects = new TypedCollection(MyObject::class);

$objects->add(new MyClass()); // all good
$objects->add(new SomeOtherClass()); // woops, type is not supported

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 see the idea but it poses me two problems:

  • it changes the initial constructor argument order (you can't use new TypedCollection([$element]) any more), which is troublesome although you could switch the two arguments to keep the array as first
  • this class is not meant to be concrete, we want to have strongly typed collection We already dropped the possibility to have strongly typed methods (explained in the issue), but I don't want to lose the strongly typed class as well If we provide a generic concrete class nobody will extend it to create real new collection classes

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, got it. :)

{
/**
* Define the type of the elements contained in the collection.
* Example: for a ProductCollection you need to return Product::class
*
* @return string
*/
abstract protected function getType();

/**
* AbstractTypedCollection constructor.
*
* @param array $elements
*
* @throws InvalidArgumentException
*/
public function __construct(array $elements = array())
jolelievre marked this conversation as resolved.
Show resolved Hide resolved
{
$this->checkElementsType($elements);
parent::__construct($elements);
}

/**
* @param mixed $element
*
* @return bool
*
* @throws InvalidArgumentException
*/
public function removeElement($element)
{
$this->checkElementType($element);

return parent::removeElement($element);
}

/**
* @param mixed $offset
* @param mixed $value
*
* @return bool|void
*
* @throws InvalidArgumentException
*/
public function offsetSet($offset, $value)
{
$this->checkElementType($value);

return parent::offsetSet($offset, $value);
}

/**
* @param mixed $element
*
* @return bool
*
* @throws InvalidArgumentException
*/
public function contains($element)
{
$this->checkElementType($element);

return parent::contains($element);
}

/**
* @param mixed $element
*
* @return bool|false|int|string
*
* @throws InvalidArgumentException
*/
public function indexOf($element)
{
$this->checkElementType($element);

return parent::indexOf($element);
}

/**
* @param mixed $key
* @param mixed $value
*
* @throws InvalidArgumentException
*/
public function set($key, $value)
{
$this->checkElementType($value);

parent::set($key, $value);
}

/**
* @param mixed $element
*
* @return bool
*
* @throws InvalidArgumentException
*/
public function add($element)
{
$this->checkElementType($element);

return parent::add($element);
}

/**
* @param array $elements
*
* @throws InvalidArgumentException
*/
protected function checkElementsType(array $elements)
{
foreach ($elements as $element) {
$this->checkElementType($element);
}
}

/**
* @param mixed $element
*
* @throws InvalidArgumentException
*/
protected function checkElementType($element)
jolelievre marked this conversation as resolved.
Show resolved Hide resolved
{
$expectedType = $this->getType();
if (!($element instanceof $expectedType)) {
throw new InvalidArgumentException(sprintf(
'Invalid element type %s, expected %s',
get_class($element),
$expectedType
));
}
}
}
35 changes: 35 additions & 0 deletions src/Core/Exception/InvalidArgumentException.php
@@ -0,0 +1,35 @@
<?php
/**
* 2007-2019 PrestaShop SA and Contributors
*
* NOTICE OF LICENSE
*
* This source file is subject to the Open Software License (OSL 3.0)
* that is bundled with this package in the file LICENSE.txt.
* It is also available through the world-wide-web at this URL:
* https://opensource.org/licenses/OSL-3.0
* If you did not receive a copy of the license and are unable to
* obtain it through the world-wide-web, please send an email
* to license@prestashop.com so we can send you a copy immediately.
*
* DISCLAIMER
*
* Do not edit or add to this file if you wish to upgrade PrestaShop to newer
* versions in the future. If you wish to customize PrestaShop for your
* needs please refer to http://www.prestashop.com for more information.
*
* @author PrestaShop SA <contact@prestashop.com>
* @copyright 2007-2019 PrestaShop SA and Contributors
* @license https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
* International Registered Trademark & Property of PrestaShop SA
*/

namespace PrestaShop\PrestaShop\Core\Exception;

/**
* Class InvalidArgumentException is thrown when the provided argument of
* a class method is invalid.
*/
class InvalidArgumentException extends CoreException
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more appropriate to rename this to TypeException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, you're right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

{
}