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

M2 Crud Model: Inspection info: invocation parameter types are not compatible with declared #476

Open
sallaberry opened this issue Aug 7, 2019 · 7 comments

Comments

@sallaberry
Copy link
Contributor

pestle magento2:generate:crud-model

Which module? (Pulsestorm_HelloGenerate)] Namespace_Module
What model name? (Thing)] Thing
Creating: /[..]/app/code/Namespace/Module/Api/ThingRepositoryInterface.php
Creating: /[..]/app/code/Namespace/Module/Model/ThingRepository.php
Creating: /[..]/app/code/Namespace/Module/Api/Data/ThingInterface.php
Creating: /[..]/app/code/Namespace/Module/Model/ResourceModel/Thing/Collection.php
Creating: /[..]/app/code/Namespace/Module/Model/ResourceModel/Thing.php
Creating: /[..]/app/code/Namespace/Module/Model/Thing.php
Creating: /[..]/app/code/Namespace/Module/Setup/InstallSchema.php
Creating: /[..]/app/code/Namespace/Module/Setup/InstallData.php

I fixed all the issues detected by phpcs / phpmd (using Magento 2 provided rulesets) like code style, missing phpdocs and other easy to fix stuff, but I have two remaining warnings in the file : /[..]/app/code/Namespace/Module/Model/ThingRepository.php

In the save and delete methods when calling respectively :

$this->objectResourceModel->save($object);

or

$this->objectResourceModel->delete($object);

Warning on $object :

Expected \Magento\Framework\Model\AbstractModel, got \Namespace\Module\Api\Data\ThingInterface
Inspection info: invocation parameter types are not compatible with declared
@astorm
Copy link
Owner

astorm commented Aug 8, 2019

@sallaberry Thank you!

Three quick questions

  1. What version of Magento are you running against?
  2. Does this only happen with the changes you made, or is it a problem with the current version of pestle.phar as well?
  3. Do you have you changes in a branch somewhere where I could look at them?

@sallaberry
Copy link
Contributor Author

  1. Magento 2.3.1
  2. Yes it happens without my changes
  3. I forked the repo and worked on a special branch if that's what you wanted : https://github.com/sallaberry/pestle/tree/%23476-Crud_Improvements

@astorm
Copy link
Owner

astorm commented Aug 9, 2019

sigh -- sounds like Magento's tightened-up/change their interface/contract assumptions in 2.3.1

@sallaberry Were you planning on submitting a PR for you phpcs / phpmd changes? If so, what would you be easier

  1. You submit your PR, we confirm it works with older versions of Magento, and then I (or we?) attack the problem of making sure the model's passed to the repository are able to pass all their type hint checks

  2. First I (or we?) attack the problem of making sure the model's passed to the repository are able to pass all their type hint checks, and then once we figure that out we make it work with what you've got on your branch?

@sallaberry
Copy link
Contributor Author

  1. Sounds good, it should still work even though I had to edit some functions in Pulsestorm\Cli\Code_Generation

I'll open the PR right now

@sallaberry
Copy link
Contributor Author

It appears the warning actually come from one of PhpStorm basic php rules, not from the official ruleset.
It also complains about SortOrder being undefined in the same file ( Model/Repository.php ).

Maybe we can ignore the warnings but PhpStorm is usually right so I'm not sure...

@astorm
Copy link
Owner

astorm commented Aug 9, 2019

@sallaberry A few more questions

  1. I think I may have misunderstood -- I thought the warning you were still seeing were generated via PHP. It sounds like you're saying that they're coming from ??some-system?? used by PHP Storm itself?

  2. Which Magento coding standards are you applying? Link/composer package?

@sallaberry
Copy link
Contributor Author

I'm sorry if I wasn't clear enough, this is just an inspection problem.

The warnings appears directly in my IDE Phpstorm which comes with his own coding standards, but if you install phpcs globally, you can point Phpstorm to the phpcs binary and use it directly from Phpstorm. (I use squizlabs/php_codesniffer installed globally)

I configured phpcs inside phpstorm to use a ruleset located in dev/tests/static/framework/Magento/ruleset.xml and phpmd to use dev/tests/static/testsuite/Magento/Test/Php/_files/phpmd/ruleset.xml

Now if there's something wrong in my code and the warning comes from phpcs or phpmd, the error message is prefixed with 'phpcs:' or 'phpmd:'. It's not the case with the two messages I have. I'm sorry I didn't notice that earlier.

Inside Phpstorm native code inspection configuration, I can disable specific rules, set their warning level etc. If I disable the rule: Type Compatibility > Parameter Type (Invocation parameter types are not compatible with declared.) then the warning on ->save($object) disappears. What I find weird is Phpstorm usually doesn't complain for nothing and I always had this rule on.

sallaberry added a commit to sallaberry/pestle that referenced this issue Aug 10, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
sallaberry added a commit to sallaberry/pestle that referenced this issue Aug 11, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
sallaberry added a commit to sallaberry/pestle that referenced this issue Aug 16, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
astorm added a commit to sallaberry/pestle that referenced this issue Aug 17, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
astorm added a commit that referenced this issue Aug 17, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
#476 crud improvements
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

No branches or pull requests

2 participants