Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make DataObject extension stricter #35

Merged

Conversation

shochdoerfer
Copy link
Member

Instead of accepting and returning mixed types, the extension now accepts and returns types based on the magic method documentation in the DataObject class.

Instead of accepting and returning mixed types, the extension now
accepts and returns types based on the magic method documentation
in the DataObject class.
@shochdoerfer
Copy link
Member Author

@hostep @sprankhub could you please check if this works relatively smoothly with your codebases? Ran it against a few of our modules and did not encounter issues. Thanks!

The general idea is to increase the strictness of the phpstan checks when DataObjects are involved. The extension adheres to the types mentioned in the doc block of the respective methods plus potential default values. But that might defer in the real world :)

baldwinagency-pieter pushed a commit to baldwin-agency/magento2-module-url-data-integrity-checker that referenced this pull request May 24, 2020
@hostep
Copy link
Contributor

hostep commented May 24, 2020

Hi @shochdoerfer, just tested this by temporarily changing my composer.json file like this:

diff --git a/composer.json b/composer.json
index d2545fd..17e7470 100644
--- a/composer.json
+++ b/composer.json
@@ -21,7 +21,7 @@
         "symfony/console": "^2.5 || ^3.0 || ^4.0"
     },
     "require-dev": {
-        "bitexpert/phpstan-magento": "^0.1.0",
+        "bitexpert/phpstan-magento": "dev-feature/strict_dataobject",
         "ergebnis/composer-normalize": "^2.2",
         "friendsofphp/php-cs-fixer": "^2.15",
         "magento/magento-coding-standard": "^5.0",
@@ -46,6 +46,10 @@
         {
             "type": "composer",
             "url": "https://repo.magento.com/"
+        },
+        {
+            "type": "vcs",
+            "url": "https://github.com/shochdoerfer/phpstan-magento"
         }
     ],
     "scripts": {

After testing it on https://github.com/baldwin-agency/magento2-module-url-data-integrity-checker it showed one more error:

------ -------------------------------------------------------------------
  Line   Cron/ScheduleJob.php
 ------ -------------------------------------------------------------------
  42     Call to an undefined method Magento\Framework\DataObject::save().
 ------ -------------------------------------------------------------------


 [ERROR] Found 1 error

I could fix that one with the following commit: baldwin-agency/magento2-module-url-data-integrity-checker@faa683c

I think it's a good addition.

@shochdoerfer
Copy link
Member Author

@hostep I wonder why this error came up. I don't think this is part of this PR, maybe due to an update of phpstan or something.

@hostep
Copy link
Contributor

hostep commented May 24, 2020

It's definitely caused by this PR, since I don't need the fix on version 0.2.0 of bitexpert/phpstan-magento and nothing else got updated.

And I think it makes sense because the getNewEmptyItem method defines DataObject as its return type and phpstan can't infer a specific datatype from it automatically. Right?

@sprankhub
Copy link
Contributor

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!

@shochdoerfer shochdoerfer merged commit 4cce4a3 into bitExpert:master May 29, 2020
@shochdoerfer shochdoerfer added this to the 0.3.0 milestone May 29, 2020
@shochdoerfer shochdoerfer deleted the feature/strict_dataobject branch August 7, 2021 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants