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

Increase PHPStan's level #1803

Merged
merged 7 commits into from
Dec 21, 2018
Merged

Increase PHPStan's level #1803

merged 7 commits into from
Dec 21, 2018

Conversation

malarzm
Copy link
Member

@malarzm malarzm commented May 19, 2018

Leftovers for level 2:

 ------ -----------------------------------------------------------------------------
  Line   lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Match.php
 ------ -----------------------------------------------------------------------------
  452    Call to an undefined method Doctrine\ODM\MongoDB\Query\Expr::maxDistance().
  474    Call to an undefined method Doctrine\ODM\MongoDB\Query\Expr::minDistance().
 ------ -----------------------------------------------------------------------------

@alcaeus I'm pretty sure I was asking you about it some time ago but I can't really remember the answer nor find anything useful on gitter...

 ------ ----------------------------------------------------------------------
  Line   lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
 ------ ----------------------------------------------------------------------
  126    Call to an undefined method Doctrine\Common\Proxy\Proxy::__wakeup().
 ------ ----------------------------------------------------------------------

Technically the method is there as it's inside of a path that checked for __wakeup existance. I was trying getting rid of it with assert(method_exists($proxy, '__wakeup') to no avail. Ideas?

To get to level 4 we need to fix Annotation's $value typehint as it hints against string while it can be anything:

 ------ --------------------------------------------------------------
  Line   lib/Doctrine/ODM/MongoDB/Mapping/Driver/AnnotationDriver.php
 ------ --------------------------------------------------------------
  65     Call to function is_array() will always evaluate to false.
  143    Call to function is_array() will always evaluate to false.
 ------ --------------------------------------------------------------

To deal with level 5 we again need to tweak Common stuff:

 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   lib/Doctrine/ODM/MongoDB/Mapping/Driver/AnnotationDriver.php
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------
  73     Parameter #1 $map of method Doctrine\ODM\MongoDB\Mapping\ClassMetadata::setDiscriminatorMap() expects array, string given.
  266    Parameter #1 $reader of class Doctrine\ODM\MongoDB\Mapping\Driver\AnnotationDriver constructor expects Doctrine\Common\Annotations\AnnotationReader,
         Doctrine\Common\Annotations\Reader given.
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------

Also I have no clue so far why PHPStan is complaining about:

 ------ ----------------------------------------------------------------------------------------------------------------------------------------
  Line   lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php
 ------ ----------------------------------------------------------------------------------------------------------------------------------------
  170    Parameter #2 $expression of method Doctrine\ODM\MongoDB\Aggregation\Expr::operator() expects
         array<Doctrine\ODM\MongoDB\Aggregation\Expr>|Doctrine\ODM\MongoDB\Aggregation\Expr, array|Doctrine\ODM\MongoDB\Aggregation\Expr given.
 ------ ----------------------------------------------------------------------------------------------------------------------------------------

@malarzm malarzm added the Task label May 19, 2018
@malarzm malarzm added this to the 2.0.0 milestone May 19, 2018
@malarzm malarzm changed the title [WIP] Get to 2nd level of PHPStan [WIP] Increase PHPStan's level May 20, 2018
@malarzm malarzm force-pushed the phpstan-l2 branch 3 times, most recently from 44832d2 to 90ecc0a Compare May 20, 2018 19:54
malarzm added a commit to doctrine/annotations that referenced this pull request May 21, 2018
`string` is wrong as it can also be `null` or an `array`. Merging this (and tagging a new bugfix release) would help me with doctrine/mongodb-odm#1803 :)
malarzm added a commit to doctrine/common that referenced this pull request May 21, 2018
Not sure about the history, but now we have an interface to "typehint" against. Also this merging this and releasing a new bugfix version will help me with doctrine/mongodb-odm#1803
malarzm added a commit to doctrine/annotations that referenced this pull request May 22, 2018
`string` is wrong as it can also be `null` or an `array`. Merging this (and tagging a new bugfix release) would help me with doctrine/mongodb-odm#1803 :)
@alcaeus
Copy link
Member

alcaeus commented May 23, 2018

Call to an undefined method Doctrine\ODM\MongoDB\Query\Expr::maxDistance().

You can remove the maxDistance and minDistance helpers. The methods in Expr were removed in 0323fb1 when we dropped geoNear commands completely in favor of the respective pipeline stage. I apparently forgot to throw them out in the match aggregation pipeline stage.

As for the undefined __wakeup method, did you try calling it conditionally, e.g. if method_exists(...) { $proxy->__wakeup();? If it still reports an error, this is something that may be an issue in PHPStan.

Last but not least, the argument type mismatch doesn't make any sense. The typehint for operator is array|self[]|self while the typehint for anyElementTrue is array|self, which should be perfectly safe for passing to operator. Again, this might be an issue with PHPStan.

Majkl578 pushed a commit to Majkl578/doctrine-persistence that referenced this pull request Jun 9, 2018
Not sure about the history, but now we have an interface to "typehint" against. Also this merging this and releasing a new bugfix version will help me with doctrine/mongodb-odm#1803
@carusogabriel
Copy link
Contributor

PHPStan 0.10 was released. Maybe we should use it here?

@malarzm
Copy link
Member Author

malarzm commented Jun 25, 2018

Heh I still need to fix stuff in other Doctrine packages :/ Maybe I can schedule some time tomorrow to get back to this, I'll see how much stuff needs to change with 0.10

@alcaeus
Copy link
Member

alcaeus commented Nov 26, 2018

@malarzm can you rebase and continue this or would you prefer me to take over?

@malarzm
Copy link
Member Author

malarzm commented Nov 27, 2018

FWIW I'm trying to rebase this

@malarzm
Copy link
Member Author

malarzm commented Nov 27, 2018

So this PR is again merge'able but I need to revisit all levels as I stopped fixing errors by 2nd one just to get through all the conflicts today. BTW @jmikola I've found interesting error that might be worth fixing in the driver (but not sure if it should so I'm not opening an issue there yet):

PHPDoc tag @throws with type MongoDB\Driver\Exception\Exception is not subtype of Throwable

@alcaeus
Copy link
Member

alcaeus commented Nov 28, 2018

I need to revisit all levels as I stopped fixing errors

I can help with some of them, I think it's worth shipping finishing this before Beta-1.

@alcaeus
Copy link
Member

alcaeus commented Nov 28, 2018

Reported the exception issue to the driver: mongodb/mongo-php-driver#940. In the meantime, we can just ignore the error entirely.

@alcaeus alcaeus force-pushed the phpstan-l2 branch 3 times, most recently from 357982c to a1f289c Compare November 28, 2018 20:54
@alcaeus alcaeus self-assigned this Dec 18, 2018
@alcaeus alcaeus changed the title [WIP] Increase PHPStan's level Increase PHPStan's level Dec 18, 2018
@alcaeus
Copy link
Member

alcaeus commented Dec 18, 2018

Regarding assertions, we should make sure assertion checks are enabled on travis-ci to make sure we're triggering an error instead of using assert only for documentation purposes in PHPStan.

@alcaeus
Copy link
Member

alcaeus commented Dec 19, 2018

I finished up fixing stuff including level 7. @malarzm I think it'd be good if you could take a look at fixes from later commits.

@alcaeus
Copy link
Member

alcaeus commented Dec 20, 2018

PR rebased to account for changes in CollectionPersister.

@malarzm
Copy link
Member Author

malarzm commented Dec 21, 2018

LGTM 👍

@malarzm malarzm merged commit 8c0c5da into doctrine:master Dec 21, 2018
@malarzm malarzm deleted the phpstan-l2 branch December 21, 2018 15:39
alcaeus pushed a commit to alcaeus/annotations that referenced this pull request May 7, 2020
`string` is wrong as it can also be `null` or an `array`. Merging this (and tagging a new bugfix release) would help me with doctrine/mongodb-odm#1803 :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants