Prestashop: Use protected over private when it's possible

[mehdi-ghezal]

Is your feature request related to a problem? Please describe.

I try to extends CartPresenter in a custom module but I cannot access property like $priceFormatter, $link, etc... because theses properties has been declare as private property.

What the purpose of private over protected in this use case ?

IMHO, it only make harder to extends Prestashop and less developer friendly.

Describe the solution you'd like

Use protected over private when it's not REQUIRED to hide property from derived class.
This recommandation can be extend in the whole project as I don't think it's only related to this class.

Describe alternatives you've considered

No alternative solution, just a workaround ; I recreate the required objects for my business logic but it's a non sense as they already exists in parent class.

Additional context

None.

[khouloudbelguith]

Hi @mehdi-ghezal,

Thanks for your report.
Ping @PrestaShop/prestashop-core-developers what do you think?
Similar issue reported on Forge: http://forge.prestashop.com/browse/BOOM-2376

Thanks!

[mehdi-ghezal]

BTW, same suggestion for methods.

[matks]

Hi @mehdi-ghezal about the answer whether to use private or protected functions/properties, the answer is probably "yes and no"

When we add new code into PrestaShop codebase, we ask ourselves the question "How should I allow PrestaShop developers to extend/customize this behavior while avoiding complex side-effects (which leads to bugs) or corrupted data state ?"

For example consider this code:

class ImageFinder
{
    private $folderPath;

    public function __construct($folderPath)
    {
        $this->folderPath = $folderPath;
    }

    public function getImageByName($imageName)
    {
        return file_get_contents($folderPath.'/'.$imageName);
    }
}

By setting this property "folderPath" in private we "lock" the class behavior and consequently we are sure the property "folderPath" CANNOT be changed between the call of __construct and the call of getImageByName. If we do not lock it, some people could do a mistake and modify the "folderPath" which could bring the following behavior:

• you call "__construct" with the right folder path => everything is cool
• some 3rd party code (maybe a module that you have installed in the shop ?) modifies the folder path
• you call getImageByName and it's broken because the folder is wrong !

Do you see what I mean ? This bug would be very hard for you to analyze because the data was changed at some point by a 3rd party code that you might not even know it's there.

This is the main reason why we put functions and properties in private. We think "this is an internal setting needed to ensure the correct behavior of this piece of code, better lock it".

In order to allow developers to extend/customize the behavior we provide protected properties and functions, public properties and functions, extension mechanisms (hooks and overrides).

But we are not always right . So YES sometimes we do a mistake and lock some setting/function that needs to be accessible from the outside as it unlocks a key customization behavior. But sometimes NO we prefer not to let this go wild as messing with some key pieces of PrestaShop can put you in a very complicated situation.

Every software/platform/library that has customization capabilities has to ask itself this complex question 😅 "should it be locked or not ?" . For example https://github.com/symfony/symfony is obviously a framework that allows extension/customization and they also have private, protected and public functions and properties 😉

To sum up

If you have some usecases where you wanted to do something with PrestaShop and you could not because of a private property/function, please tell us by opening a github issues 😉 (or even creating the Pull Request to make it available ?) we'll consider the change and if it is relevant we'll change it for the next version. But sometimes we'll keep some things locked as we are afraid of the dangerous mistakes their usage can bring.

[mehdi-ghezal]

Hello,

I get it about the encapsulation principle and the idea about ensure that the object behave as excepted ; but let me give you an example : CartPresenter.php

This class is used different place such as CartController. There is no possibilities to override it globally (and override is bad !!). So it mean, the CartController will ALWAYS use the CartPresenter define in Prestashop namespace.

If you look about this class, almost everything is private, so in my module I want to reuse this class by extend it ; but I cannot access the property such as $priceFormatter. I have to create a new instance of PriceFormatter ; same problem with methods.

So, this class has been locked with no apparent reason as :

1. We cannot override it globally (AFAIK)
2. It's use explicitly in controller (cannot change the class by an IoC mechanism for example)

In this suggestion, I talk about inheritance and not overriding, it's not the first time I get this kind of problem and it's not in legacy code but new one. It's why I made this suggestion.

One last thing, I understand that you trying to protect the core from wrong modules or poor development but with too much restriction people will just :

• Edit the core file to change (ouch...)
• Copy / Paste the complete code that could be easily break in a future upgrade

I know, there is no magic solution and not one single answer, but when I look the CartPresenter class I cannot hold myself to think about a big black box where everything has been put in private without a real thinking.

Regards,

[matks]

@mehdi-ghezal About the Cart Presenter:

Unfortunately the people who built it are no longer working on the project, so we will not have the answers for sure, however my guess would be this:

By "locking" the Cart Presenter, you provide one guarantee for 3rd-party themes: "we guarantee the following list of variables will always be available for you to use in your Smarty templates". That's the only relevant reason I can think of. If we allow people to modify its behavior, then it means 3rd-party theme might not work on a modified shop as the required variable might not be available.

This put aside, your comment is right:

This class is used different place such as CartController. There is no possibilities to override it globally (and override is bad !!). So it mean, the CartController will ALWAYS use the CartPresenter define in Prestashop namespace.

So would the following changes address your need ?

• allow dependency injection on CartPresenter in order to allow you to use another PriceFormatter
• allow dependency injection on CartController in order to allow you to use another CartPresenter

I think it would make sense, and dependency injection is a proper way to perform the wanted behavior.

[khouloudbelguith]

Hi @mehdi-ghezal,

Since we had no news from you for more than 30 days, I'll close this ticket. Feel free to open another one if you can give specific details.

Thanks!

[jf-viguier]

@mehdi-ghezal is right about the Cart Presenter : we can't change it it's a main issue. Why closing it @khouloudbelguith ?

[jf-viguier]

Presenter should at list have hooks :
https://github.com/PrestaShop/PrestaShop/issues/11125

[okom3pom]

Bump !

[khouloudbelguith]

Hi @jf-viguier,

About closing the issue after 30 days with no response, we just discussed in this subject & it will be an update of those rules described in this issue: https://github.com/PrestaShop/prestashop-retro/issues/54
In the meantime, I will re-open this issue.

is right about the Cart Presenter: we can't change it it's a main issue.

Ping @PrestaShop/prestashop-core-developers what do you think?

Thanks!

[PierreRambaud]

Since #11125 has been merged, we don't have to use protected instead of private right now.
Feel free to reopen if you think it's really mandatory.