search

extdn / extdn-phpcs

PHP CodeSniffer rules
81 stars 10 forks source link

Rule: ->getResource() in Model #14

Open lbajsarowicz opened 6 years ago

lbajsarowicz commented 6 years ago

Most of the Mageto 2 tutorials and stackoverflow answers:

  1. https://www.mage2.tv/content/setup-scripts/adding-a-custom-eav-attribute/adding-an-input-filter-to-a-customer-or-address-attribute/
  2. https://magento.stackexchange.com/questions/163933/good-practice-how-to-save-many-objects
  3. https://magento.stackexchange.com/questions/46369/loading-just-an-attribute-from-a-model-and-saving-it

provide getResource()->save($object) as recommended way to persist data changes on model - from model itself. As use of getResource() is deprecated, and ResourceModel should be initialized using Dependency Injection, we should introduce inspection for PHP CS to avoid using getResource() in models and setup scripts.

jissereitsma commented 6 years ago

Wowser! Yes, this definitely seems like a cool rule.

fooman commented 6 years ago

Not sure how feasible it would be to extend the check to use repositories where they exist, ie $quoteRepository->save($quote).

schmengler commented 6 years ago

@fooman I think it's enough to document repositories as suggested alternative if they exist and provide the needed method. Resource models will then be listed as fallback.

Question: how likely are false positives? Can getResource() refer to something else than the deprecated method from the abstract Magento model class?

fooman commented 6 years ago

@schmengler I think the narrower we can make the rule the better to avoid false positives (also easier to document). I believe this rule covers descendants of Magento\Framework\Model\AbstractModel only and in that context getResource() would always be a correct positive (unless you somehow overwrite parent behaviour but that would be odd as well).

There is a range of deprecation annotations here https://github.com/magento/magento2/blob/2.2-develop/lib/internal/Magento/Framework/Model/AbstractModel.php so we might need multiple rules.

schmengler commented 6 years ago

So it boils down to "If class extends Magento\Framework\Model\AbstractModel, then $this->getResource(), $this->_getResource() and $this->getResourceCollection() should not be used" :+1: