0

I have two repositories:

class RepositoryOne
{
    /**
     * @param int $id
     * @return ModelOne
     */
    public function getById($id)
    {
        // Search and find a ModelOne model in the db.
        return new ModelOne();
    }
}

class RepositoryTwo
{
    /**
     * @param int $id
     * @return ModelTwo
     */
    public function getById($id)
    {
        // Search and find a ModelTwo model in the db.
        return new ModelTwo();
    }
}

I am a skilled developer so I program to an interface. So let's refactor it to this:

interface RepositoryOneInterface
{
    /**
     * @param int $id
     * @return ModelOne
     */
    public function getById($id);
}

class RepositoryOne implements RepositoryOneInterface
{
    /**
     * @inheirtDoc
     */
    public function getById($id)
    {
        // Search and find a ModelOne model in the db.
        return new ModelOne();
    }
}

// Same goes for ModelTwo

Turns out the logic to return a model is exactly the same. Also I have not two repositories, but seven. So I create an abstract repository interface:

interface AbstractRepositoryInterface
{
    /**
     * @param int $id
     * @return AbstractModel
     */
    public function getById($id);
}

I've also added abstract repository implementation so that it is not repeated in each repository. So the whole thing looks like this:

class AbstractModel {}

class ModelOne extends AbstractModel {}

interface AbstractRepositoryInterface
{
    /**
     * @param int $id
     * @return AbstractModel
     */
    public function getById($id);
}

interface RepositoryOneInterface extends AbstractRepositoryInterface { }

abstract class AbstractRepository implements AbstractRepositoryInterface
{
    /**
     * @inheirtDoc
     */
    public function getById($id)
    {
        //$modelResource = $this->getModelResource();
        // $model = $modelResource->load();
        // return $model;
    }

    protected abstract function getModelResource();
}

class RepositoryOne extends AbstractRepository implements RepositoryOneInterface
{
    protected function getModelResource()
    {
        // return ModelOne Resource.
    }
}

Now I can reuse my abstract repository class for every repository I have (seven). The problem is that I don't have a concrete return type now (after the refactor). So if I do this:

$repositoryOne = new RepositoryOne();
$repositoryOne->getById(5);

...sure I get an instance of ModelOne; but the program sees it as an AbstractModel. If I use intellisense, I get only methods of the AbstractModel; but my repository is clearly a RepositoryOne one!

What is the right way to solve this issue so that the program perceives the return value as a ModelOne (without an explicit use of annotation on the returned variable) while maintaining the use of inheritance (not composition)

3
  • 3
    Just because two things have the same interface doesn't mean you should extract a base class. Jan 12 at 23:30
  • In Swift, it is actually not very likely that different types with the same interface (called "protocol" in Swift) have a common base class.
    – gnasher729
    Jan 13 at 0:30
  • 2
    You say "use of inheritance", but it's not clear what you're using inheritance for. What would be the problem if you simply deleted the interface and the abstract class, and let each repository stand alone?
    – bdsl
    Jan 13 at 0:47

1 Answer 1

5

Turns out the logic to return a model is exactly the same. Also I have not two repositories, but seven. So I create an abstract repository interface:

interface AbstractRepositoryInterface
{
   /**
    * @param int $id
    * @return AbstractModel
    */
   public function getById($id);
}

There's your problem.

You shouldn't create an interface simply because things work the same way - interfaces are for things can be used interchangeably by code that depends on them. That depending code can reference the interface and not the concrete class.

For instance an interface would be appropriate to share between something like DatabaseBasedRepositoryOne and InMemoryRepositoryOne - because to the calling code they are interchangeable and they just save the data in a different place.

RepositoryOne and RepositoryTwo are not interchangeable, so they should not share an interface.

8
  • Thank you for your answer. I think I understand the issue. I just want to rephrase the question based on your answer: does it mean that whenever the return type is not concrete enough then it is always an incorrect use of inheritance?
    – pro100tom
    Jan 13 at 9:19
  • I'm not sure, I think the main point is that any code element should only exist if it has a purpose, especially when that code causes a problem. An interface should normally have something that uses it (e.g. as a property and parameter type, not in an implements clause) to show what it's purpose is.
    – bdsl
    Jan 13 at 9:22
  • 1
    In PHP 7.4 and later there is also a 'return type covariance feature', which allows an implementation to return a subtype of what the interface returns. But I'd still ask what the purpose of the interface is first.
    – bdsl
    Jan 13 at 9:24
  • 1
    @pro100tom Yes, assuming that each Model extends AbstractModel then that's legal, and complies with the Liskov principle - and in PHP 7.4 and above you could do it with native return types instead of docblocks. But what's it for? Is there any place in the code where you would need to call the function AbstractRepositoryInterface::getById - i.e. you have a variable or property typed as AbstractRepositoryInterface and you invoke the function on it?
    – bdsl
    Jan 13 at 9:33
  • 1
    OK - if you're not calling getById in an abstract way I'd suggest there isn't a good reason to abstract it out. But I'm not familiar with Magento, maybe someone who's worked with it or looked at it in detail will be able to give you more specific advice.
    – bdsl
    Jan 13 at 9:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.