Add deprecation rules for AbstractModel classes

[shochdoerfer]

Access to save(), load(), delete() and getCollection() is deprecated.

[ondrejmirtes]

Writing tests for these rules is also really easy, you need to extend RuleTestCase. It looks like this: https://github.com/phpstan/phpstan-src/blob/master/tests/PHPStan/Rules/Exceptions/CaughtExceptionExistenceRuleTest.php

[shochdoerfer]

@ondrejmirtes thanks for your valuable feedback and your work on phpstan, obviously :)

[ondrejmirtes]

Found a few more things :)

[shochdoerfer]

@hostep @sprankhub could you both also please test this against your modules? Would love to know how much breaks :) Thanks!

[hostep]

Hi @shochdoerfer

These are the error I'm getting right now on https://github.com/baldwin-agency/magento2-module-url-data-integrity-checker/tree/d71e782f7e570ba30d2ab2b2f2c460967eb429bb:

 ------ -----------------------------------------------------------------------------
  Line   Cron/ScheduleJob.php
 ------ -----------------------------------------------------------------------------
  42     Use service contracts to persist entities in favour of mixed::save() method
 ------ -----------------------------------------------------------------------------

 ------ ------------------------------------------------------------------------------------------------------------
  Line   Storage/CacheStorage.php
 ------ ------------------------------------------------------------------------------------------------------------
  29     Use service contracts to persist entities in favour of Magento\Framework\App\CacheInterface::save() method
  34     Use service contracts to persist entities in favour of Magento\Framework\App\CacheInterface::load() method
 ------ ------------------------------------------------------------------------------------------------------------

I don't think there are service contracts available in Magento for these 2 things (and probably a bunch of others as well)?

[ondrejmirtes]

if ($isAbstractModelType->no()) { should be changed to if (!$isAbstractModelType->yes()) {

[hostep]

That seems to solve the reported issues indeed, thanks @ondrejmirtes! (I had to run vendor/bin/phpstan clear-result-cache however before the change was picked up, which is slightly annoying.)

I'm a bit worried about that previous mixed::save() report however, in reality it's the Magento\Cron\Model\Schedule::save() and Magento\Cron\Model\Schedule does inherit from Magento\Framework\Model\AbstractModel, so in this case it should still report the error if I understand it correctly.
So it looks like the exact type isn't getting inferred correctly here. Code: https://github.com/baldwin-agency/magento2-module-url-data-integrity-checker/blob/d71e782f7e570ba30d2ab2b2f2c460967eb429bb/Cron/ScheduleJob.php#L40-L47

[shochdoerfer]

@ondrejmirtes can you explain the difference to me? Is it because of maybe() ?

[shochdoerfer]

@hostep thanks for your analysis. I'll check where the problem with mixed::save() is coming from.

[ondrejmirtes]

@shochdoerfer You need to imagine types in a venn diagram. The largest circle (which contains all the types) is mixed - which is the top type. So mixed is super type of all the types.

If one smaller circle is inside a bigger circle (like mixed <> int relationship), then mixed is super type of int (yes()), but if you ask for an inverse relationship, it's maybe().

Same for objects. Exception is super type of all the Exception implementations (yes()). But InvalidArgumentException is maybe() a super type of Exception.

If two types are totally unrelated (like string and int), then isSuperType returns no().

What happened in your case:

(new ObjectType('Magento\Framework\Model\AbstractModel'))->isSuperTypeOf($type)

Abstract model might be a super type of mixed.

And CacheInterface must be somehow related to AbstractModel, it's its parent or something like that...

[shochdoerfer]

@ondrejmirtes thx, your insights are very much appreciated!

[sprankhub]

Same here: Just tested this. The new issues I get through this seem to be caused by the phpstan/phpstan update from 0.12.19 to 0.12.25. Hence, all good from my side. Thanks for your work!

However, if this PR raises errors for all usages of save, load etc., I have strong objections against that. Because as @hostep already pointed out, there are many entities, which are not covered with proper APIs yet (e.g. product custom options).

[shochdoerfer]

@sprankhub I get your point. Would it make sense to add a whitelist configuration of classes or namespaces that should be excluded in this check? Not sure about how many classes we are talking about, though. @hostep what do you think?

I would really love to establish a few rules but I fear the current state of Magento that does not allow that without dozens of exceptions :(

[sprankhub]

I fear a whitelist is hard to do, because it is also version-specific... But yeah, if it is easy to whitelist specific classes / namespaces in the phpstan.neon, I would be fine with it.

[hostep]

A configurable whitelist in the phpstan.neon file sounded like a good idea to me at first.

But then I realised we can already add exceptions by default with phpstan using something like:

parameters:
    ignoreErrors:
        - '/Use service contracts to persist entities in favour of Magento\\Framework\\App\\CacheInterface\:\:(?:save|load)\(\) method/'

So it's not really necessary since it's boiling down to the same thing I guess?

[ondrejmirtes]

But then I realised we can already add exceptions by default

That's not a good idea because if someone doesn't have that code in their app, it would get marked as unused ignore... It's a better idea to add a whitelist parameter to the rule itself.

[shochdoerfer]

@sprankhub the problem with the Magento version is already a problem since the stubs/mock I added to fix a few "doc block" issues are not compatible anymore with older Magento versions (2.0 and 2.1 I think). Since we now would rely on rules that have been introduced by a specific Magento version, I have to add the minimum magento/framework version as a required dependency, I guess. I would have loved to avoid that to reduce maintenance, but I guess there's no way around it.

@hostep I'll check the logic once again. The cache interface clearly does not extend AbstractModel, thus that error should not come up. I feel there's still a logic problem somewhere that lists way too many errors. If that is solved we might not need a whitelist by default.

[shochdoerfer]

Updated the rules logic with all the hints provided by Ondřej. Ran tests against a log of open source Magento modules and the only problem I was running into was the problem mentioned by @hostep in regards of Magento\Cron\Model\Schedule::save(). Technically, there's a resource model for the Schedule class which could be used to save the objects. So, I'd guess the error is fine.

To avoid some "noise" in some circumstances, I added 2 config options to be able to disable both rules independently. This plus the possibility to ignore errors via PHPStan should be "enough" to configure the extension to behave like you want.

We still could add a whitelist later if it makes sense.

[shochdoerfer]

Thanks for all your help with this and your insights. Would love to add more rules in the future. Feel free to open new issues if you have ideas.

[ondrejmirtes]

@shochdoerfer Instead of adding options to rules themselves to turn them off by not reporting anything, instead you can use the conditionalTags section like here: https://github.com/Roave/no-floaters/blob/master/rules.neon#L10-L12

[shochdoerfer]

Ah nice, even better. Thanks!