Sylius: Impossible to properly override MoneyHelper

[vcraescu]

Sylius/src/Sylius/Bundle/CoreBundle/Templating/Helper/MoneyHelper.php

    private $currencyContext;

    private $moneyFormatter;

These 2 properties need to be protected not private because it's impossible to override this helper when needed.

[okwinza]

They should stay private. If you need to override MoneyHelper you can do it via service container config.

https://github.com/Sylius/Sylius/blob/67bdeca081d11443e334bb9e9075e8e1b4246d07/src/Sylius/Bundle/CoreBundle/Resources/config/templating.xml#L20

And you don't have to extend the base one. Just copy the code and override that parameter line.

[vcraescu]

So if this class is later extended I'll just copy paste the new functionality? That's what you suggest?
Any reason for this strong encapsulation?

[dorumd]

@okwinza And if in a new release extended class is changed, I should always copy paste it? Seems to a be a little bit weird.

[okwinza]

Actually, i have reviewed the class once more and i couldn't find a reason why some properties were made private. @pjedrzejewski any thoughts?'

My input above should still serve as a temporary workaround tho.

[pjedrzejewski]

We are leaning towards composition over inheritance for most customizations, so you either need to decorate this http://symfony.com/doc/current/components/dependency_injection/advanced.html#decorating-services or provide your own implementation. Could you share your use-case for customizing this class? Maybe it is something simple that we can add in our implementation in Sylius itself.

[vcraescu]

@pjedrzejewski makes sense to lean towards composition over inheritance but those injected dependencies should at least have some getters so I don't inject them again.
Composition over inheritance comes with a cost and doesn't make sense in all the cases. In the end we might end up rewrite a lot of logic for the sake of programming toward an interface.

The change we need to do is tailored to our needs and probably doesn't make sense to any other project.

[pamil]

@craescu if that change does not make any sense for any other project, it does make sense not to extend that class and write your own implementation. The existing implementation contains only three lines.

There are plenty of ways to customise the behaviour without the need to extend that class: decorating existing implementation, implementing a new helper or injecting different collaborators to the existing one.

I'd say we rather should make this class final in the first place as after stable release we won't be able to change the API provided by protected variables or methods because of the backwards compatibility promise.

[lchrusciel]

It is always a good moment to remind a good presentation: Extremely Defensive PHP

+1 for final + private

[vcraescu]

OK guys. :)