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

phpstan fixes #7

Closed
weierophinney opened this issue Dec 31, 2019 · 11 comments
Closed

phpstan fixes #7

weierophinney opened this issue Dec 31, 2019 · 11 comments

Comments

@weierophinney
Copy link
Member

This PR fixes some issues with phpdoc and types. Used phpstan to static analyse the code.

Added phpstan in CI build.


Originally posted by @thomasvargiu at zendframework/zend-servicemanager#272

@weierophinney
Copy link
Member Author

I've added phpstan in CI build and fixed some wrong uses of array_key_exists with ArrayObject.


Originally posted by @thomasvargiu at zendframework/zend-servicemanager#272 (comment)

@weierophinney
Copy link
Member Author

@thomasvargiu I've just noticed that this targets master: can you change it to develop? Since it's an improvement, it needs to target the next minor or major.


Originally posted by @Ocramius at zendframework/zend-servicemanager#272 (comment)

@weierophinney
Copy link
Member Author

Please rebase, don't merge :)


Originally posted by @Ocramius at zendframework/zend-servicemanager#272 (comment)

@weierophinney
Copy link
Member Author

My fault, I'm still working on it


Originally posted by @thomasvargiu at zendframework/zend-servicemanager#272 (comment)

@weierophinney
Copy link
Member Author

@Ocramius
I have just one issue that I want to fix:

------ ----------------------------------------------------------------------------------------------------------------------------
  Line   src/AbstractPluginManager.php
 ------ ----------------------------------------------------------------------------------------------------------------------------
  130    Parameter #1 $instance of method Zend\ServiceManager\AbstractPluginManager::validate() expects object, array|object given.
 ------ ----------------------------------------------------------------------------------------------------------------------------

I think we should change PHPDoc of $instance parameter in Zend\ServiceManager\PluginManagerInterface::validate() from object to mixed because we know that services config key can contain anything.

But I'm not sure about this change. We can also ignore the error at the moment.

What do you think about it?


Originally posted by @thomasvargiu at zendframework/zend-servicemanager#272 (comment)

@weierophinney
Copy link
Member Author

I've noted that phpstan should only be added for Travis jobs that require it, not as a general dev dependency.

Done.

Additionally, I'll note that a large number of the changes presented here are present in other pull requests; you'll likely need to rebase and run again once those are in place.

It's not a problem, when I open it I though that it could be useful before to introduce strict type hints. We can merge other PRs and then rebase it.


Originally posted by @thomasvargiu at zendframework/zend-servicemanager#272 (comment)

@weierophinney
Copy link
Member Author

Rescheduling to 4.0.0 since it targets develop in turn targeting 4.0


Originally posted by @Xerkus at zendframework/zend-servicemanager#272 (comment)

@weierophinney
Copy link
Member Author

@weierophinney I rebased it, updated phpstan, done some other fixes.

I also allowed array in the PluginManager validate() method.


Originally posted by @thomasvargiu at zendframework/zend-servicemanager#272 (comment)

@GeeH
Copy link
Contributor

GeeH commented Jun 5, 2020

@laminas/technical-steering-committee is PHPStan fixes valid now or are we going all in with Psalm in the future? Do we see value in pull these changes across please?

@GeeH
Copy link
Contributor

GeeH commented Jun 8, 2020

Ping @MWOP @Ocramius

@samsonasik
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants