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

Add phpstan #29

Closed
wants to merge 6 commits into from
Closed

Add phpstan #29

wants to merge 6 commits into from

Conversation

alexmanno
Copy link
Member

No description provided.

@alexmanno alexmanno requested a review from Jean85 October 11, 2018 15:54
.travis.yml Outdated
@@ -14,6 +14,9 @@ matrix:
include:
- php: 7.1
env:
- TEST_PHPSTAN=true
Copy link
Member

Choose a reason for hiding this comment

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

Why to instantiate another PHP 7.1 environment?

.travis.yml Outdated
@@ -41,6 +42,7 @@ install:

script:
- if [[ $TEST_COVERAGE ]]; then phpdbg -qrr bin/phpunit --coverage-clover clover.xml; else bin/phpunit; fi;
- if [[ $TEST_PHPSTAN ]]; then php ./bin/phpstan analyse -l 7 src fi;
Copy link
Member

Choose a reason for hiding this comment

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

We should analyse tests directory too. I think it would be better to add the phpstan configuration and required extensions if necessary

@@ -14,17 +14,20 @@
*/
class Connection extends DBALConnection
{
/** @var ServerGoneAwayExceptionsAwareInterface */
protected $_driver;
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't override this property just to assign a different phpdoc.

@@ -129,7 +133,7 @@ public function query()
* @param array $params
* @param array $types
*
* @return integer The number of affected rows.
* @return integer|null The number of affected rows.
Copy link
Member

Choose a reason for hiding this comment

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

This method should not return null

@@ -45,7 +45,7 @@ private function createStatement()
/**
* @param array|null $params
*
* @return bool
* @return bool|null
Copy link
Member

Choose a reason for hiding this comment

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

This method should not return null

Copy link
Member

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Build still fails due to PHP 5.6 still supported. We can either bump to 7.1 (and do a new major) or add PHPStan on the fly during the build.

@peelandsee
Copy link
Member

@Jean85 @alexmanno

php5.6 will be supported (security) until 31 Dec 2018.
php7.0 will be supported (security) until 3 Dec 2018.

i would prefer to keep compatibility up to those dates.

when we'll bump the minimum php version will be also useful to update the README in order to provide a compatibility version matrix.

thanks again for this pr!

@peelandsee
Copy link
Member

@alexmanno
are you still interested in completing this pr?

@peelandsee
Copy link
Member

@alexmanno may you have a look at this?
are you still interested in fixing this pr?

@peelandsee
Copy link
Member

no answer from @alexmanno,
closing this now

@peelandsee peelandsee closed this Sep 3, 2019
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

4 participants