Skip to content
This repository has been archived by the owner on Feb 6, 2020. It is now read-only.

phpstan fixes #272

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

thomasvargiu
Copy link
Contributor

@thomasvargiu thomasvargiu commented Oct 13, 2018

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

Added phpstan in CI build.

@@ -933,7 +934,7 @@ private function validateOverrides(array $config)
* service instances; if not, it returns, but otherwise, it raises an
* exception indicating modification is not allowed.
*
* @param string[] $services
* @param array $services
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd stick with string[].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, my mistake 👍

@thomasvargiu
Copy link
Contributor Author

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

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Ocramius Ocramius self-assigned this Oct 19, 2018
@Ocramius
Copy link
Member

@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.

@Ocramius Ocramius assigned thomasvargiu and unassigned Ocramius Oct 19, 2018
@Ocramius Ocramius added this to the 3.4.0 milestone Oct 19, 2018
@thomasvargiu thomasvargiu changed the base branch from master to develop October 19, 2018 12:15
@Ocramius
Copy link
Member

Please rebase, don't merge :)

@thomasvargiu thomasvargiu changed the title phpstan fixes WIP: phpstan fixes Oct 19, 2018
@thomasvargiu
Copy link
Contributor Author

My fault, I'm still working on it

@thomasvargiu
Copy link
Contributor 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?

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

composer.json Outdated
@@ -29,6 +29,7 @@
"mikey179/vfsStream": "^1.6.4",
"ocramius/proxy-manager": "^2.1.1",
"phpbench/phpbench": "^0.13.0",
"phpstan/phpstan": "^0.10.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be added programmatically in the .travis.yml when PHPSTAN_TEST is selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@thomasvargiu
Copy link
Contributor 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.

@Xerkus Xerkus modified the milestones: 3.4.0, 4.0.0 Dec 20, 2018
@Xerkus
Copy link
Member

Xerkus commented Dec 20, 2018

Rescheduling to 4.0.0 since it targets develop in turn targeting 4.0

@thomasvargiu thomasvargiu changed the title WIP: phpstan fixes phpstan fixes Mar 19, 2019
@thomasvargiu
Copy link
Contributor Author

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

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

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-servicemanager; a new issue has been opened at laminas/laminas-servicemanager#7.

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-servicemanager. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-servicemanager to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-servicemanager.
  • In your clone of laminas/laminas-servicemanager, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

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

Successfully merging this pull request may close these issues.

5 participants