Return types in core make it hard to overwrite methods


#1

Hi there,

for some (recently updated?) methods in the Spryker core, there are return types specified.
While this generally is a good thing, it makes it hard to impossible to overwrite those methods with an extended interface as PHP does not allow to change the method’s signature.

Example:
Spryker/Glue/GlueApplication/GlueApplicationFactory.php:
public function createRestResourceBuilder(): RestResourceBuilderInterface
{
return new RestResourceBuilder();
}

On project level we cannot have a child of RestResourceBuilderInterface as new return type.

Sometimes it works to just specify a different return type in the annotations, but that leads to inconsistency which is criticized by the code sniffer.

Maybe it would make sense to not use return types in the core to keep the methods overwritable?


#2

Hi!

May I ask what you are actually trying to achieve? Because there a but few cases where it really makes sense to extend an interface. Maybe we can come up with a more suitable approach!

Cheers


#3

Hi lazystar,

I’ll give a more detailed example of what we try to achieve.

Our goal was to extend the RestResponseInterface to add an additional method that’s available for our RestResponses on project level.

To do so, we’ve overwritten the createRestResponse method from RestResourceBuilder in Glue/GlueApplication/Rest/JsonApi. It now returns a new RestResponse whose Interface is also extended on project level.

Due to the specified return type in the core for RestResourceBuilderInterface and it’s implementation, we cannot announce our extended RestResponseInterface as the new return type (although it includes all the methods from the core Interface). So in PHP we have to add the parent Interface as return type, although we know that we’ll return a child Interface at that time.

As a second example, where we can overwrite as needed is the AbstractFactory in Glue/Kernel.
Here the getResourceBuilder() method doesn’t have a strict return type, but works only with annotations.
That way, we can easily overwrite the annotation (or even add a return type) in our extendend AbstractFactory in Pyz.

Best regards
Felix


#4

I did a little more research on this topic and found the original PHP RFC: https://wiki.php.net/rfc/return_types#variance_and_signature_validation

There it’s documented that the return type is invariant and therefore must not be changed, even in inheritance scenarios like ours.

The RFC also says that this might be possible in the future. But for now, the only way to overwrite the return type on project level is to omit it in the core.

Best regards
Felix


#5

Exactly. But in a world of desired statelessness (unfortunately the Response might be an exception here), there is usually no need to extend an interface. Thus my question about your goal.

If you could give me some insight into what this method is used for, we might find another approach t achieve this goal!


#6

In this particular case, we wanted to extend the RestResponseInterface by a method to handle caching directives like setting a Cache-Control header.

As an alternative we’ve discussed some kind of (static) helper class, we can pass the Response object onto. But this would probably come with other downsides like more difficult unit tests.


#7

Hi lazystar,

did you have time to think about another practical solution for this problem?

To be honest, I didn’t get your statement about the statelessness and extension of interfaces.
Why is it “bad” to extend an interface on project level?

Best regards
Felix


#8

Sorry for the repost, but here’s yet another example from a business factory:

\Spryker\Zed\Product\Business\ProductBusinessFactory::createProductConcreteStatusChecker

 /**
 * @return \Spryker\Zed\Product\Business\Product\Status\ProductConcreteStatusCheckerInterface
 */
public function createProductConcreteStatusChecker(): ProductConcreteStatusCheckerInterface
{
    return new ProductConcreteStatusChecker($this->getRepository());
}

If we want to extend the ProductConcreteStatusChecker on project level, we cannot use the original factory method to replace it with our own implementation, although we completely support the original interface.

Imagine the need of both replacing an existing function (so also core classes use the new implementation) and adding a new interface method.
In this case we need to maintain two factory methods that return the same implementation with different return type.


#9

Hi!

The two scenarios we have here are a bit different.

  1. Extending the response is a bit more trickier, because the response itself is carrying state (please let me know if you want me to elaborate here). I would have to have a deeper look into where you want to add the header and where it would become relevant. I think there was an example somewhere of how to wrap the response. Maybe a look at \Spryker\Glue\GlueApplication\Rest\Cors\CorsResponse can already help.

  2. You see it completely correct, that you would have to have two factory methods with the same class for different interfaces. After all, the factory returns implementations (the classes) for the hinted abstractions (the interfaces). And ever so often these might be the same at the moment, but might become two entirely different classes/implementations further down the road.
    Extending a class, only because it looks like you can re-use bits of it, might be a hint of a violation of the Single Responsibility Principle.

Maybe you want to share a bit more about your scenario here and please let me know if you want me to elaborate on any aspect above!

Cheers


#10

Hi lazystar,

thanks for your response!

  1. I’ll have a look at CorsResponse, it actually looks promising! :slight_smile:

About my second concern:
I think I get your point.
So your advice is to overwrite core classes only if it’s for changing the way a specific, existing function works? Every additional functionality (that would also result in a change of the interface) should be represented by a new interface/implementation then, correct?

I think there are still some (maybe rare) cases where it’s legit to actually extend the core classes, like EntityRepositories. By using return types you currently do not leave the choice to the developers, but this might be on purpose, I guess.

Good to mention: a RFC for support of covariant parameter and return types was accepted in January: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters
So it’s only a matter of time until this won’t be an issue anymore.

Best regards
Felix


#11

Interfaces such as the RepositoryInterfaces you can actually extend on project level. Since it is auto-resolved it will automatically load the version that is higher in the classname hierarchy. In you factories you just have to put the correct @method annotation.