Fix decoration issues with simultaneous decoration of AccountOrderCon…

[mstegmeyer]

The Shopware commercial plugin also decorates the same controller. For this to work (though not supposed to be done, because StorefrontControllers are @internal and therefore only to be changed by Shopware), this change will at least not make the container fail.

[mstegmeyer]

In combination with Shopware's Commercial plugin, this is not compatible because if the decoration order has yours prioritized over Shopware's, it will break, because Shopware's decoration expects an AccountOrderController, not a StorefrontController.

In general, I would always expect that the decorating class extends the decorated class and not some parent class. Exceptions are available Interfaces / Abstract classes, which is not the case for these internal StorefrontControllers.

[rommelfreddy]

Hello @mstegmeyer thank you for your contribution.

we know, that this decoration is not correct/perfect. This is caused by that Shopware's AccountOrderController does not have an abstract parent or better an interface for this controller. By Symfony pattern it is allowed to extend the original class. In my opinion it does not make sense to extend the original class, because you should never call any parent methods - so the parent will never be used.

So this the reason why we decided to use the storefront-controller as parent, because this is the safest parent, which should always match. Long story short: the problem is: it doesn't matter how we implement the decoration for this controller - each solution is the wrong solution because the AccountOrderController does not have a abstract parent or an interface.

So if you want to make sure, that your plugin is compatible with it and use a better decoration, you can change the parent to the storefront-controller which is much safer. (this requires that all other plugins implement it on the same way)

We are currently discuss if we can completely remove this decoration.

[mstegmeyer]

Removing the decoration would truly be the better solution, since all StorefrontControllers are tagged as @internal and should therefore not be extended or decorated.

I would disagree with your opinion on which class to extend. When decorating, I would always expect to see the decorated class as the type, except if there is an abstract class or interface obvious for decorating. Why would you choose StorefrontController? With your logic, you could also use AbstractController. Also, yes, you are not calling parent, but you are calling methods on your decorated service (so e.g. $this->decorated->editOrder()), and when using a StorefrontController, this method would not reliably exist and a static analysis like PhpStan would throw errors.

Also, this is not about my personal plugin, this is about Shopware's own, most important plugin, the Commercial plugin with all its commercial features.

[pixelecho]

PayOne please add these pull request. Why such a discussion when this simple pull request will fix the whole problem?

[rommelfreddy]

sorry for the long process. We ware trying to have a wide compatibility with the module. We are sorry, that will break some other modules.

We discussed the controller decoration and decided to remove the decoration completely, because it is not longer required.

in the future, we will set the payment status to authorized, after the payment has been complete, so it is not possible to change the payment method after the order has been placed. This was the reason why the decoration was necessary in the past.

So, thank you, @mstegmeyer, very much for your contribution.

In the future it is not required anymore, so i will close this PR. I expect the change in the next release.