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

Updates phpstan version and fixes newly discovered bugs #319

Open
wants to merge 2 commits into
base: 4.7
from

Conversation

Projects
None yet
4 participants
@bytehead
Copy link
Member

bytehead commented Feb 6, 2019

No description provided.

@@ -30,7 +30,7 @@ class DoctrineMigrationsDiffCommand extends DiffCommand
/**
* {@inheritdoc}
*/
public function execute(InputInterface $input, OutputInterface $output): void
public function execute(InputInterface $input, OutputInterface $output): ?int

This comment has been minimized.

@@ -67,7 +67,7 @@ public function hasToken($tokenId): bool
/**
* {@inheritdoc}
*/
public function removeToken($tokenId): void
public function removeToken($tokenId): ?string

leofeyer added a commit to contao/phpstan that referenced this pull request Feb 6, 2019

Require phpstan/phpstan ^0.11 (#2)
Description
-----------

Should give a new release: `0.5.0`. See also following PR: contao/contao#319

Commits
-------

3be903a Require phpstan/phpstan ^0.11
$this->usedTokens[$tokenId] = true;
$this->tokens[$tokenId] = null;
if (array_key_exists($tokenId, $this->tokens)) {

This comment has been minimized.

@leofeyer

leofeyer Feb 6, 2019

Member

Why did you add this?

This comment has been minimized.

@leofeyer

leofeyer Feb 6, 2019

Member

@aschempp @ausi According to the interface, we are supposed to return string|null. However, we are not returning anything at the moment.

  1. How can this have worked?
  2. Are the changes correct in your opinion?

This comment has been minimized.

@ausi

ausi Feb 6, 2019

Member
  1. How can this have worked?

Returning nothing is the same as null so there was no type error. We don’t use the return value of this method in our code so it probably didn’t have an effect for our use case.

  1. Are the changes correct in your opinion?

Yes, not returing the token was a mistake :(
We should probably test the return value in the unit test:

$memoryTokenStorage->removeToken('foo');
$this->assertFalse($memoryTokenStorage->hasToken('foo'));
$this->assertSame(['foo' => null, 'baz' => 'bar'], $memoryTokenStorage->getUsedTokens());
$memoryTokenStorage->removeToken('baz');

This comment has been minimized.

@aschempp

aschempp Feb 7, 2019

Contributor

Completely agree with @ausi

This comment has been minimized.

@leofeyer

leofeyer Feb 7, 2019

Member

I have backported the fixes in 66803d6.

@leofeyer leofeyer added the defect label Feb 6, 2019

@leofeyer leofeyer added this to the 4.7.0 milestone Feb 6, 2019

"contao/test-case": "^2.0",
"doctrine/doctrine-migrations-bundle": "^1.1",
"doctrine/event-manager": "^1.0",
"doctrine/orm": "^2.5",
"friendsofphp/php-cs-fixer": "^2.12",
"monolog/monolog": "^1.22",
"php-coveralls/php-coveralls": "^2.1",
"phpstan/phpstan-phpunit": "^0.10",
"phpstan/phpstan-phpunit": "^0.11",

This comment has been minimized.

@leofeyer

leofeyer Feb 6, 2019

Member

Why is this necessary?

Edit: nvm, I found out myself. 😁

This comment has been minimized.

@bytehead

bytehead Feb 6, 2019

Author Member

https://getcomposer.org/doc/articles/versions.md#caret-version-range-

The ^ operator behaves very similarly but it sticks closer to semantic versioning, and will always allow non-breaking updates. For example ^1.2.3 is equivalent to >=1.2.3 <2.0.0 as none of the releases until 2.0 should break backwards compatibility. For pre-1.0 versions it also acts with safety in mind and treats ^0.3 as >=0.3.0 <0.4.0.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 6, 2019

Since intersection type support is still buggy in PhpStorm (see https://youtrack.jetbrains.com/issue/WI-45275), I would prefer not to merge this into the 4.7 branch. WDYT?

@leofeyer leofeyer added feature and removed defect labels Feb 6, 2019

@leofeyer leofeyer modified the milestones: 4.7.0, 4.8.0 Feb 6, 2019

leofeyer added a commit that referenced this pull request Feb 7, 2019

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