Use private instead of protected visibility

[joshdifabio]

In Magento 1, class properties and methods are all defined with public or protected visibility. In PHP, changing the API of a protected property or method usually constitutes a backwards compatibility break. Have Magento considered using private instead of protected visibility as the default alternative to public in Magento 2?

Consider the following examples:

namespace Magento;

// version 1.0
class Product
{
    protected function somethingInternal() {}
}

namespace SomeThirdParty;

class MyProduct extends \Magento\Product
{
    public function somethingPublic()
    {
        // ...
        parent::somethingInternal();
        // ...
    }
}

namespace SomeOtherThirdParty;

class MyProduct extends \Magento\Product
{
    public function someCoolNewMethod() {}
}

namespace Magento;

// version 1.1
class Product
{
    // this breaks SomeThirdParty\MyProduct
    protected function somethingInternal($newParam) {}

    // this breaks SomeOtherThirdParty\MyProduct
    protected function someCoolNewMethod($requiredParam) {}
}

The above examples demonstrate how adding or modifying protected methods can break downstream packages. If those methods had been declared private, re-factoring would have been fine without any BC issues.

Why use protected by default? Presumably it's 'just in case' someone downstream wants to replace or re-use a piece of functionality. The thing to understand is that protected comes with a cost; it essentially means you can't do any re-factoring. If a piece of functionality is really worth exposing then why isn't it public?

[vrann]

Hi, Joshua,
my name is Eugene Tulika, I’m an architect in Magento. Thanks a lot for your question, it is something which we were debating a lot while working on Magento 2. Some part of my answer may be opinionated.

When you choose visibility scope for you method, the main question to answer is how the method is going to be used. In most cases it is more or less clear what should be public — usually it is everything which fulfill single responsibility of the class.

The main question to answer is should my method be private or protected. The easiest way to go here is to make it protected:

• it doesn’t lock the class to later to modification of behavior via inheritance and allows to re-use the code in similar classes.
• it is just “safer" to open everything: in the open-source world it is very hard to predict what your customers would like to subclass and modify, you operate in the environment where you don’t know all the potential use-cases.
• in Magento 1.x it was a code standard to use protected visibility for all the core classes in order to allow them be rewritten in the community extensions. We have a lot of legacy code with all the methods protected just for this reason.

However you are right that it comes for the cost: you should keep backward compatibility of the protected methods and moreover, you facilitate inheritance and potential overrides in the places where it may be not the best choice to do.

In Magento 2, with introduction of dependency injection and installment of best practices, the approach which we were taking for some of our components is “composition over inheritance”: if you think something should be protected — move it to the separate class, make it public and inject to the class which going to use it as a constructor dependency. In this way, you keep all of your dependencies clear, you open a way to change the behavior without subclassing and you can re-use the code. Besides, this enforces single responsibility of the class and helps when you write unit tests. From this perspective everything should be either public or private.

These are general considerations which should be taken when code is designed. Now I want to address backward compatibility part of this question particularly for Magento 2. With introduction of Service Contracts, you should call just methods which belongs to Service Contracts interfaces and avoid calling methods which are not part of it. If this rule is followed, it doesn’t matter a lot is method private or protected. If it’s not part of service contracts, there is no guarantee for the backward compatibility, even if it is public.

[joshdifabio]

@vrann, thanks so much for your in-depth response, it's greatly appreciated.

Regarding service contracts: is the aim for dependencies between core modules to be entirely isolated to the functionality exposed by service contracts? If not, then presumably the service contracts don't expose quite enough functionality to be considered an adequate 'public API' for a module, and so it might not be realistic to expect solution providers and third party extension developers to limit their dependencies on Magento to the service contracts. I.e., if Magento's core modules will depend on protected methods of other core modules, is it realistic to expect that third parties won't have to do the same? The same applies to public methods which aren't part of a service contract.

If the service contracts truly provide a sufficient API then that's great. If not, do they not lose a lot of their value? And if that's the case and SemVer is only guaranteed for those service contracts, then does SemVer not also lose a lot of its value?

Regarding the favouring of composition over inheritance for new functionality; that's great and I'm delighted to hear that you guys are going that way.

Thanks again for your hard work and for taking the time to reply to me!

[joshdifabio]

I've just had a look at the service contracts for catalog, sales and customer for the first time in a month or so: wow, they look a lot more extensive now! I think you can disregard this issue now and just continue the good work. Thanks for your help :).

[vrann]

You are right, the goal of service contracts is to have core modules to depend on functionality exposed by service contracts interfaces only. Expectation for the third party extensions is the same.

Currently we are in process of achieving this goal, as you noticed contracts for Customer, Sales, Checkout, Tax, Catalog, CatalogInventory evolved a lot within last few months. However you still may find dependencies from core extension on the methods which are not exposed via the contracts. The process of refactoring these dependencies will take some time after the contracts are created. In Customer and Tax case though, you can see that the modules which were depending on them was refactored to use just service contracts.

Thanks for the feedback:)

[joshdifabio]

On second thoughts, I'm gonna re-open this issue if that's okay; I've noticed that the framework code is also full of protected properties & methods and I'm assuming that those packages won't define service contracts. I don't expect you to comment further at this point in time, but I think it'd be good for other people to be able to add their thoughts here if they so wish.

[maksek]

Closing the issue, since discussion is closed. For further discussions please open topic in forum.

[likemusic]

Full of protected properties & methods and I'm assuming that those packages won't define service contracts.

After 3 years it's still here. Should all that in result become private and internally calls another injected classes that implements some service contracts?

[likemusic]

Sometime when developing extensions for magento I find issues in nested private method ($class->publicMathod()->privateMethod1()->privateMethod2()->buggyPrivateMethod3()). I'm talking about both php classes and js components. To fix nested buggy private method I should copy-paste many lines of code starting from public method to nested buggy. And after all new releases I should take care about copy-pasted methods if its not changed.

I think that make this nested private methods as protected would be less evil then thoughtless using private methods everywhere referring to Technical guidelines.

Am i wrong?

[magento-engcom-team]

Hi @likemusic
GitHub issue tracker for magento2 repository is not intended for discussions. We are closing this ticket now.

Please refer to the Community Forums or the Magento Stack Exchange site for advice or general discussion about this issue.