Magento2: Logical error: method getRequestUri() is absent in \Magento\Framework\App\RequestInterface but widely used in Magento 2 code (which depends on a pacticular implementation \Magento\Framework\App\Request\Http)

Created on 14 Aug 2015  路  11Comments  路  Source: magento/magento2

All 11 comments

internal issue MAGETWO-43626

This is quite a big issue. Controllers almost _never_ assume \Magento\Framework\App\RequestInterface when they use the Request object. Understandable: that interface doesn't help much.
I hope you guys don't come back closing this ticket just because there's no feedback. :)

I agree that is an important issue. There are many places where the core code relies on methods provided by concrete implementations that are not part of the interface (the controller result objects are just one more example).
I'm always unsure if that is not yet refactored legacy code and there are plans to decompose the class into separate interfaces or some similar approach, or if it is considered already final.
In the latter case the methods should be added to the interface, since the clients obviously need to know about them.
Maybe after the final release someone from Magento could give insight into what the plan is to tackle this problem.

+1

Would like to point out another highly used method from \Magento\Framework\App\Request\Http is the getFullActionName() method.

I agree that is an important issue. There are many places where the core code relies on methods provided by concrete implementations that are not part of the interface (the controller result objects are just one more example).

True.

We can write a test to find all such places and, for instance, enforce such situation does not appear in any new code but with BC constraints we would have to deprecate HUGE amount of code to fix such issues in existing code.

Without a concrete implementation approach such issue existence in GitHub does not make a lot of sense. Otherwise it is a good candidate for 'up for grabs'.

Thoughts?

I understand the desire to keep ticket count down, but I think it's good to leave this open even if we don't yet have a concrete solution. So long as it's an acknowledged issue (as it is), it's good to have a canonical ticket to track progress toward a solution. Otherwise it will keep getting reported over and over, and any discussion will get diluted unhelpfully.

As for a possible suggestion, would it break BC too much to introduce a new interface that is more specific than RequestInterface and includes methods from Http? The methods that currently just specify RequestInterface but expect Http could be updated to use this interface instead. Even if this could only be applied to non-API methods, it would solve most of the existing issues without involving a massive bulk deprecation.

would it break BC too much to introduce a new interface that is more specific than RequestInterface and includes methods from Http?

This in my eyes would be a possible way to allow for my proposed changes in issue #10481 to be BC.

This is not about issue count but about unactionable items. We should either figure out some direction for a comprehensive solution within a reasonable time frame or admit it is impractical in existing constraints and, maybe, introduce mitigation plan only (like protect new code from similar issue).

Please get familiar with http://devdocs.magento.com/guides/v2.1/contributor-guide/backward-compatible-development/ to have an idea what kind of changes is allowed. Method argument type, unlike the exception type, cannot be changed even if new interface is a sub-type of old one.

more specific than RequestInterface and includes methods from Http?

I don't see a big difference for customization if we put all public Http methods to a brand new interface or just use it directly.

Thanks @orlangur.

Good info on the dev docs page regarding backwards compatible changes. I appreciate the feedback.

Hi @dmitry-fedyuk
Due to Backward compatibility policy the method getRequestUri() can not be added to RequestInterface. But we have MAGETWO-43626 in our backlog. As a possible solution we will deprecate this interface and introduce new one.

Was this page helpful?
0 / 5 - 0 ratings