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

Conversation

Projects
None yet
6 participants
@jolelievre
Copy link
Contributor

jolelievre commented Jan 9, 2019

Questions Answers
Branch? 1.7.6.x
Description? Implement an abstract collection that check the type of inserted elements.
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #12065
How to test? Nothing to test

This change is Reviewable

@jolelievre jolelievre added this to the 1.7.6.0 milestone Jan 9, 2019

@jolelievre jolelievre added this to In progress in PrestaShop 1.7.6 via automation Jan 9, 2019

@jolelievre jolelievre force-pushed the jolelievre:abstract-collection branch from 2ce2529 to 374cdb1 Jan 9, 2019

@jolelievre jolelievre removed this from In progress in PrestaShop 1.7.6 Jan 9, 2019

@prestonBot prestonBot added the Feature label Jan 9, 2019

@mickaelandrieu
Copy link
Contributor

mickaelandrieu left a comment

Good job!

Show resolved Hide resolved src/Core/Data/AbstractTypedCollection.php Outdated

@eternoendless eternoendless changed the title Implements AbstractTypedCollection with tests Implement AbstractTypedCollection with tests Jan 10, 2019

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

jolelievre commented Jan 10, 2019

@PierreRambaud @mickaelandrieu if it's ok for you I think me can merge it (once travis is done)
It will be useful for the Mail template generator PR

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

This comment has been minimized.

@sarjon

sarjon Jan 11, 2019

Member

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

This comment has been minimized.

@jolelievre

jolelievre Jan 11, 2019

Author Contributor

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

This comment has been minimized.

@sarjon

sarjon Jan 11, 2019

Member

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

This comment has been minimized.

@jolelievre

jolelievre Jan 11, 2019

Author Contributor

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

This comment has been minimized.

@sarjon

sarjon Jan 11, 2019

Member

ok, got it. :)

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

This comment has been minimized.

@eternoendless

eternoendless Jan 11, 2019

Member

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

This comment has been minimized.

@jolelievre

jolelievre Jan 11, 2019

Author Contributor

yep, you're right

This comment has been minimized.

@jolelievre

jolelievre Jan 11, 2019

Author Contributor

done!

@mickaelandrieu mickaelandrieu merged commit 9884eb9 into PrestaShop:develop Jan 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Jan 11, 2019

Thanks @jolelievre !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment