Welcome! Log In Create A New Profile

Advanced

[PHP] How do I handle covariant parameters and not fall foul of LSP.

Posted by Richard Quadling 
Hi.

I have an abstract base repo and an abstract base entity. Both provide
functionality
appropriate to all repos/entities.

I extend these to allow a specific repo to handle a specific entity.
But I can't
declare the specific entity type hint in the specific repo as it is in
conflict.

I'm hitting my head against LSP, but wanting
https://en.wikipedia.org/wiki/Covariance_and_contravariance_
(computer_science)#Contravariant_method_argument_type

<?php

abstract class BaseEntity{}

abstract class BaseRepo
{
public function processEntity(BaseEntity $entity){}
}

class Person extends BaseEntity{}

class PersonRepo extends BaseRepo
{
public function processEntity(Person $entity)
{
parent::processEntity($entity);
}
}

$person = new Person;
$personRepo = new PersonRepo;
$personRepo->processEntity($person);


Warning: Declaration of PersonRepo::processEntity(Person $entity)
should be compatible
with BaseRepo::processEntity(BaseEntity $entity)

I know some people 'get' this. But for me, it really seems to make next to no
sense at all.

I have some base behaviour for all entities. I have a base repo that can work
with that base entity, regardless of how that entity is extended.

If the base type were not abstract, then I can see that I wouldn't substitute
anything logically, but I'm using abstract base types. And if set theory is
to be believed, then if B extends A, B is still an A. And so, where I can
receive an A, I can receive a B.

Obviously, I'm missing / lacking an understanding of LSP as it relates to my
code.

I'm happy to read more, but I'd appreciate some recommendations on what to
read.

Of course, if anyone can 'correct' my code such that a concrete repo works on
concrete entities, but also allows for the base behaviour, then I'd be very
grateful.

Regards,

Richard.
I know it might look like deflecting the issue but maybe this could work

abstract class BaseRepo
{
protected function doProcessEntity(BaseEntity $entity){}
}

class PersonRepo extends BaseRepo
{
public function processEntity(Person $entity)
{
$this->doProcessEntity($entity);
}
}

On Mon, Feb 13, 2017 at 5:13 PM, Richard Quadling <[email protected]>
wrote:

> Hi.
>
> I have an abstract base repo and an abstract base entity. Both provide
> functionality
> appropriate to all repos/entities.
>
> I extend these to allow a specific repo to handle a specific entity.
> But I can't
> declare the specific entity type hint in the specific repo as it is in
> conflict.
>
> I'm hitting my head against LSP, but wanting
> https://en.wikipedia.org/wiki/Covariance_and_contravariance_
> (computer_science)#Contravariant_method_argument_type
>
> <?php
>
> abstract class BaseEntity{}
>
> abstract class BaseRepo
> {
> public function processEntity(BaseEntity $entity){}
> }
>
> class Person extends BaseEntity{}
>
> class PersonRepo extends BaseRepo
> {
> public function processEntity(Person $entity)
> {
> parent::processEntity($entity);
> }
> }
>
> $person = new Person;
> $personRepo = new PersonRepo;
> $personRepo->processEntity($person);
>
>
> Warning: Declaration of PersonRepo::processEntity(Person $entity)
> should be compatible
> with BaseRepo::processEntity(BaseEntity $entity)
>
> I know some people 'get' this. But for me, it really seems to make next to
> no
> sense at all.
>
> I have some base behaviour for all entities. I have a base repo that can
> work
> with that base entity, regardless of how that entity is extended.
>
> If the base type were not abstract, then I can see that I wouldn't
> substitute
> anything logically, but I'm using abstract base types. And if set theory is
> to be believed, then if B extends A, B is still an A. And so, where I can
> receive an A, I can receive a B.
>
> Obviously, I'm missing / lacking an understanding of LSP as it relates to
> my
> code.
>
> I'm happy to read more, but I'd appreciate some recommendations on what to
> read.
>
> Of course, if anyone can 'correct' my code such that a concrete repo works
> on
> concrete entities, but also allows for the base behaviour, then I'd be very
> grateful.
>
> Regards,
>
> Richard.
>
On 13 February 2017 at 16:36, Stefan A. <[email protected]> wrote:

> I know it might look like deflecting the issue but maybe this could work
>
> abstract class BaseRepo
> {
> protected function doProcessEntity(BaseEntity $entity){}
> }
>
> class PersonRepo extends BaseRepo
> {
> public function processEntity(Person $entity)
> {
> $this->doProcessEntity($entity);
> }
> }
>
> On Mon, Feb 13, 2017 at 5:13 PM, Richard Quadling <[email protected]>
> wrote:
>
>> Hi.
>>
>> I have an abstract base repo and an abstract base entity. Both provide
>> functionality
>> appropriate to all repos/entities.
>>
>> I extend these to allow a specific repo to handle a specific entity.
>> But I can't
>> declare the specific entity type hint in the specific repo as it is in
>> conflict.
>>
>> I'm hitting my head against LSP, but wanting
>> https://en.wikipedia.org/wiki/Covariance_and_contravariance_
>> (computer_science)#Contravariant_method_argument_type
>>
>> <?php
>>
>> abstract class BaseEntity{}
>>
>> abstract class BaseRepo
>> {
>> public function processEntity(BaseEntity $entity){}
>> }
>>
>> class Person extends BaseEntity{}
>>
>> class PersonRepo extends BaseRepo
>> {
>> public function processEntity(Person $entity)
>> {
>> parent::processEntity($entity);
>> }
>> }
>>
>> $person = new Person;
>> $personRepo = new PersonRepo;
>> $personRepo->processEntity($person);
>>
>>
>> Warning: Declaration of PersonRepo::processEntity(Person $entity)
>> should be compatible
>> with BaseRepo::processEntity(BaseEntity $entity)
>>
>> I know some people 'get' this. But for me, it really seems to make next
>> to no
>> sense at all.
>>
>> I have some base behaviour for all entities. I have a base repo that can
>> work
>> with that base entity, regardless of how that entity is extended.
>>
>> If the base type were not abstract, then I can see that I wouldn't
>> substitute
>> anything logically, but I'm using abstract base types. And if set theory
>> is
>> to be believed, then if B extends A, B is still an A. And so, where I can
>> receive an A, I can receive a B.
>>
>> Obviously, I'm missing / lacking an understanding of LSP as it relates to
>> my
>> code.
>>
>> I'm happy to read more, but I'd appreciate some recommendations on what to
>> read.
>>
>> Of course, if anyone can 'correct' my code such that a concrete repo
>> works on
>> concrete entities, but also allows for the base behaviour, then I'd be
>> very
>> grateful.
>>
>> Regards,
>>
>> Richard.
>>
>
>
That's what I'm having to do. Just seems really odd.
The problem with LSP is that every BaseRepo must be able to be substituted
for any other, but each concrete form only handles its own concrete entity
type. The contract of processEntity(BaseEntity) is "I will process any
concrete BaseEntity subclass", but PersonRepo violates this contract by
declaring that it will only process PersonEntity.

Java uses Generics to solve this, but you'll have to fake it with
instanceof for safety. Note that you're *still* violating LSP here, but you
can't really avoid it given what you want to do.

class PersonRepo extends BaseRepo
{
public function processEntity(BaseEntity $entity)
{
parent::processEntity($entity);
if (!($entity instanceof Person) {
throw new RepoException('Must be a Person');
}
...
}
}

Well, you could invert the responsibility to save LSP by moving the
concrete functionality into the entity, but it will probably be a little
hacky.

abstract class BaseEntity {
public abstract function process();
}

class PersonRepo extends BaseRepo
{
public function processEntity(BaseEntity $entity)
{
parent::processEntity($entity);
$entity->process();
...
}
}

Cheers!
David
I think LSP becomes an issue in situations where you want to swap
implementations. Since I can't really think of such a use case when using
repositories, you can even name your methods more specifically.

abstract class BaseRepo
{
protected function processEntity(BaseEntity $anEntity)
{
...
}
}

class PersonRepo extends BaseRepo
{
public function processPerson(PersonEntity $aPerson)
{
$this->processEntity($aPerson);
}
}

class CatRepo extends BaseRepo
{
public function processCat(CatEntity $aCat)
{
$this->processEntity($aCat);
}
}

If the process method is doing storage related things I don't think is a
good idea to move it to the Entity hierarchy. The main idea of repositories
is to provide a collection like interface to storage, the other idea common
to persistence related patterns is to free entities from persistence
responsibilities and make them more focused on the domain logic.



On Mon, Feb 13, 2017 at 8:55 PM, David Harkness <[email protected]>
wrote:

> The problem with LSP is that every BaseRepo must be able to be substituted
> for any other, but each concrete form only handles its own concrete entity
> type. The contract of processEntity(BaseEntity) is "I will process any
> concrete BaseEntity subclass", but PersonRepo violates this contract by
> declaring that it will only process PersonEntity.
>
> Java uses Generics to solve this, but you'll have to fake it with
> instanceof for safety. Note that you're *still* violating LSP here, but you
> can't really avoid it given what you want to do.
>
> class PersonRepo extends BaseRepo
> {
> public function processEntity(BaseEntity $entity)
> {
> parent::processEntity($entity);
> if (!($entity instanceof Person) {
> throw new RepoException('Must be a Person');
> }
> ...
> }
> }
>
> Well, you could invert the responsibility to save LSP by moving the
> concrete functionality into the entity, but it will probably be a little
> hacky.
>
> abstract class BaseEntity {
> public abstract function process();
> }
>
> class PersonRepo extends BaseRepo
> {
> public function processEntity(BaseEntity $entity)
> {
> parent::processEntity($entity);
> $entity->process();
> ...
> }
> }
>
> Cheers!
> David
>
On 14 February 2017 at 05:52, Stefan A. <[email protected]> wrote:

> I think LSP becomes an issue in situations where you want to swap
> implementations. Since I can't really think of such a use case when using
> repositories, you can even name your methods more specifically.
>
> abstract class BaseRepo
> {
> protected function processEntity(BaseEntity $anEntity)
> {
> ...
> }
> }
>
> class PersonRepo extends BaseRepo
> {
> public function processPerson(PersonEntity $aPerson)
> {
> $this->processEntity($aPerson);
> }
> }
>
> class CatRepo extends BaseRepo
> {
> public function processCat(CatEntity $aCat)
> {
> $this->processEntity($aCat);
> }
> }
>
> If the process method is doing storage related things I don't think is a
> good idea to move it to the Entity hierarchy. The main idea of repositories
> is to provide a collection like interface to storage, the other idea common
> to persistence related patterns is to free entities from persistence
> responsibilities and make them more focused on the domain logic.
>
>
>
> On Mon, Feb 13, 2017 at 8:55 PM, David Harkness <[email protected]
> > wrote:
>
>> The problem with LSP is that every BaseRepo must be able to be substituted
>> for any other, but each concrete form only handles its own concrete entity
>> type. The contract of processEntity(BaseEntity) is "I will process any
>> concrete BaseEntity subclass", but PersonRepo violates this contract by
>> declaring that it will only process PersonEntity.
>>
>> Java uses Generics to solve this, but you'll have to fake it with
>> instanceof for safety. Note that you're *still* violating LSP here, but
>> you
>> can't really avoid it given what you want to do.
>>
>> class PersonRepo extends BaseRepo
>> {
>> public function processEntity(BaseEntity $entity)
>> {
>> parent::processEntity($entity);
>> if (!($entity instanceof Person) {
>> throw new RepoException('Must be a Person');
>> }
>> ...
>> }
>> }
>>
>> Well, you could invert the responsibility to save LSP by moving the
>> concrete functionality into the entity, but it will probably be a little
>> hacky.
>>
>> abstract class BaseEntity {
>> public abstract function process();
>> }
>>
>> class PersonRepo extends BaseRepo
>> {
>> public function processEntity(BaseEntity $entity)
>> {
>> parent::processEntity($entity);
>> $entity->process();
>> ...
>> }
>> }
>>
>> Cheers!
>> David
>>
>
>

I'm trying to move to generators for the entities and repos, so I think
having :

protected BaseRepo::persistEntity(BaseEntity $entity): Entity {
// Store the entity based upon required elements from the concrete repo
(table name, primary key, that sort of thing).
// Get a clean copy of the
}

and then have the generator create (sort of thing - without guards, etc.) :

PersonRepo::persist(Person $person):Person{ return
parent::processEntity($person);}
PersonRepo::find(int $personID):Person{ return new
Person(parent::findByID($personID));}

will be the way to go. Every public/concrete repo has a persist() and
find() method suitable for the entity type which proxies to the BaseRepo.

I think the next issue is that I want to enforce the persist() and find()
methods.

I suppose if I'm using a generator, then the concrete classes will be
black-box and as much boilerplate as needed, so ... maybe all moot really.

Thanks for confirming I wasn't going mad with LSP. I still can't see why an
extended BaseEntity is still not a BaseEntity, just a more specialised
variant.

If anyone has some good examples (real world ideally) where LSP is sane AND
includes inheritance (they seem to be where the conflict lies) then I may
be able to learn why I'm doing it wrong.

Regards,

Richard.
Christoph M. Becker
Re: [PHP] How do I handle covariant parameters and not fall foul of LSP.
February 14, 2017 12:10PM
On 14.02.2017 at 11:20, Richard Quadling wrote:

> If anyone has some good examples (real world ideally) where LSP is sane AND
> includes inheritance (they seem to be where the conflict lies) then I may
> be able to learn why I'm doing it wrong.

https://www.gnu.org/software/sather/docs-1.2/tutorial/type-conformance.html
and
http://www1.icsi.berkeley.edu/~sather/Documentation/EclecticTutorial/node15.html
may be helpful.

Also note, that PHP currently only supports parameter invariance (not
contravariance).

--
Christoph M. Becker

--
PHP General Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
On Tue, Feb 14, 2017 at 2:20 AM, Richard Quadling <[email protected]>
wrote:

> If the process method is doing storage related things I don't think is a
> good idea to move it to the Entity hierarchy.
>

I totally agree. I was just showing a way to avoid violating LSP.

On Tue, Feb 14, 2017 at 2:20 AM, Richard Quadling <[email protected]>
wrote:

> I still can't see why an extended BaseEntity is still not a BaseEntity,
> just a more specialised variant.
>

Because you're changing the contract in PersonEntity::processEntity() from
taking any BaseEntity to accepting only PersonEntity, you cannot substitute
any BaseEntity for it.

If anyone has some good examples (real world ideally) where LSP is sane AND
> includes inheritance (they seem to be where the conflict lies) then I may
> be able to learn why I'm doing it wrong.
>

A typical example is multiple cache backend implementations: CacheFile and
CacheMemcache. Swapping between the two requires no changes other than the
initial setup. The user of the cache shouldn't know or care which it has.

Cheers!
David
On 14 February 2017 at 20:31, David Harkness <[email protected]>
wrote:

> On Tue, Feb 14, 2017 at 2:20 AM, Richard Quadling <[email protected]>
> wrote:
>
>> If the process method is doing storage related things I don't think is a
>> good idea to move it to the Entity hierarchy.
>>
>
> I totally agree. I was just showing a way to avoid violating LSP.
>
> On Tue, Feb 14, 2017 at 2:20 AM, Richard Quadling <[email protected]>
> wrote:
>
>> I still can't see why an extended BaseEntity is still not a BaseEntity,
>> just a more specialised variant.
>>
>
> Because you're changing the contract in PersonEntity::processEntity() from
> taking any BaseEntity to accepting only PersonEntity, you cannot substitute
> any BaseEntity for it.
>
> If anyone has some good examples (real world ideally) where LSP is sane
>> AND includes inheritance (they seem to be where the conflict lies) then I
>> may be able to learn why I'm doing it wrong.
>>
>
> A typical example is multiple cache backend implementations: CacheFile and
> CacheMemcache. Swapping between the two requires no changes other than the
> initial setup. The user of the cache shouldn't know or care which it has.
>
> Cheers!
> David
>

That's fine for concrete classes, but abstract classes which cannot be
instantiated, therefore cannot be swapped, cannot fall foul of LSP. It is
the abstract part that, in my mind, should be making the difference.


Obviously, if someone makes an abstract class non-abstract, LSP will kick
in, but that is an odd thing to do in a framework.


I'm happy to leave this alone as I think I'm not truly seeing the issue
with abstract classes and LSP in the way I'm wanting to implement them.
I've gone with the proxy approach - which is fine.

Thank you all for your comments.

Regards,

Richard.
"Richard Quadling" wrote in message
news:[email protected]m...
>
<snip>
>
>That's fine for concrete classes, but abstract classes which cannot be
>instantiated, therefore cannot be swapped, cannot fall foul of LSP. It is
>the abstract part that, in my mind, should be making the difference.

That is correct. LSP does not come into play when you inherit from an
abstract class, only when you inherit from one concrete class to create a
different concrete class. The author of LSP should have made this clear.

--
Tony Marston


--
PHP General Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
On Sun, Feb 19, 2017 at 1:56 AM, Tony Marston <[email protected]>
wrote:

> "Richard Quadling" wrote in message news:CAKUjMCXEXqy=VB8Jjjg8XKdU
> sc5NsqQTeZcs54+V5e4GH1RYYw@mail.gmail.com...
>
>>
>> <snip>
>
>>
>> That's fine for concrete classes, but abstract classes which cannot be
>> instantiated, therefore cannot be swapped, cannot fall foul of LSP. It is
>> the abstract part that, in my mind, should be making the difference.
>>
>
> That is correct. LSP does not come into play when you inherit from an
> abstract class, only when you inherit from one concrete class to create a
> different concrete class. The author of LSP should have made this clear.


I believe LSP should apply to all class hierarchies. If you have a List
interface and supply an AbstractList partial implementation, all
classes—concrete or not—that implement List or extend AbstractList should
be substitutable for all others. LinkedList, ArrayList, SubList, etc.

What Richard is encountering is a breakdown due to PHP not having support
for generics. PersonRepository is a subclass of Entity and breaks LSP
because we'd rather have Repository<Person>. So he's not really violating
LSP in spirit, only in practice because PHP.

Cheers!
David
Sorry, only registered users may post in this forum.

Click here to login