magento / marketplace-eqp

Magento 1.x Coding Standard
http://docs.magento.com/marketplace/user_guide/Resources/pdf/Extension_Quality_Program_Overview.pdf
MIT License
224 stars 68 forks source link

Inherited from magento 2 core properties and "Use of protected class members is discouraged." #84

Closed likemusic closed 5 years ago

likemusic commented 6 years ago

Magento 2 have very much classes with protected class members that should be overridden in extension's child classes. Therefor there is no sense to show this message for inherited from Magento-namespace classes, because developers can't change his access-visibility.

Therefor it would be great to have some command line option to disable this warning for inherited from magento properties and functions.

lenaorobei commented 6 years ago

@likemusic for Magento 2.x development we recommend using the principle of Composition over inheritance. Closing this issue as duplicate #7 You can find more information here and here.

likemusic commented 6 years ago

@lenaorobei looks like you not completely understand what I mean.

For example if we have code like below:

use Magento\Framework\Model\ResourceModel\Db\Collection\AbstractCollection;

class Collection extends AbstractCollection
{
    protected $_idFieldName = ProductPuqConfigInterface::ID;
    protected $_eventPrefix = 'aitoc_productunitsandquantities_order_collection';
    protected $_eventObject = 'order_collection';

    protected function _construct()
    {
        $this->_init(
            OrderItemModel::class,
            OrderResourceModel::class
        );
    }
}

It that developers MUST to define field as protected (or public) because they inherited from Magento core classes. Developers can't use private here until it not changed in base magento class. Therefor warnings in that case is meaningless.

lenaorobei commented 6 years ago

Thanks for the example. We will think how to make messages more descriptive and how to exclude findings for such cases like this.

likemusic commented 6 years ago

One more message related to that issue: Property name "$_eventPrefix" should not be prefixed with an underscore to indicate visibility

diazwatson commented 6 years ago

Don't you think that those protected methods in the core should not be there?

Yes, right now the only way to use them is to extend them but that doesn't mean those methods will no be refactored and become public or even better the core team will create some API to use those objects in future releases.

scolandrea commented 6 years ago

Sorry but this is insane! For example, if we create a model class we are not following standards.

Which is the correct approach in this case scenario?

sreichel commented 6 years ago

Check underscore mehtod names is correct, but thre should be a wihitelist for names that are used in core in cannot be changed due to backward compatiblity.

lenaorobei commented 5 years ago

Closing this issue because this repo now contains sniffs for Magento 1.x code only. Please refer to magento/magento-coding-standard for Magento 2.x coding standard.

This rule was removed in magento/magento-coding-standard in order to eliminate false-positive findings.