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 QA checks through PHPStan and Psalm #413

Merged
merged 1 commit into from
Oct 28, 2021
Merged

Conversation

phansys
Copy link
Member

@phansys phansys commented Jul 24, 2021

Depends on #414 and #415.

Changelog

### Fixed
- Return value at `TokenStorageUsernameCallable::__invoke()`.

### Changed
- Several docblock types detected by PHPStan.

To Do

  • Add missing hints;
  • Re-visit several generics;
  • Add a changelog note.;
  • Fix Psalm issues under tests/ directory.

@FlintCIBot
Copy link

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/64084

This comment was posted by FlintCI. It can be disabled in the repository settings.

@FlintCIBot
Copy link

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/64085

This comment was posted by FlintCI. It can be disabled in the repository settings.

@FlintCIBot
Copy link

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/64088

This comment was posted by FlintCI. It can be disabled in the repository settings.

@FlintCIBot
Copy link

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/64090

This comment was posted by FlintCI. It can be disabled in the repository settings.

@FlintCIBot
Copy link

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/64104

This comment was posted by FlintCI. It can be disabled in the repository settings.

@@ -64,6 +64,9 @@ class AuditReader
* Entity cache to prevent circular references.
*
* @var array
*
* @phpstan-template T of object
Copy link
Member

Choose a reason for hiding this comment

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

A template on a property seems weird, it's on the class or on a method generally.
All the @phpstan-template T of object might be moved to the class...

Comment on lines 24 to 26
* @phpstan-template TKey
* @psalm-template TKey of array-key
* @psalm-template T
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need different template for phpstan and psalm ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was copied from Collection.

@FlintCIBot
Copy link

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/64105

This comment was posted by FlintCI. It can be disabled in the repository settings.

@FlintCIBot
Copy link

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/64106

This comment was posted by FlintCI. It can be disabled in the repository settings.

@@ -17,6 +17,8 @@ class ChangedEntity
{
/**
* @var string
*
* @phpstan-var class-string<T> $className
Copy link
Member

Choose a reason for hiding this comment

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

Tu use T, u have to defining the template on the class, and not only the construct i think

@FlintCIBot
Copy link

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/64108

This comment was posted by FlintCI. It can be disabled in the repository settings.

@FlintCIBot
Copy link

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/64109

This comment was posted by FlintCI. It can be disabled in the repository settings.

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

VincentLanglet
VincentLanglet previously approved these changes Oct 27, 2021
@@ -34,6 +35,8 @@ abstract class BaseTest extends TestCase
protected static $conn;

/**
* NEXT_MAJOR: Use `\Doctrine\ORM\EntityManagerInterface` instead.
Copy link
Member

Choose a reason for hiding this comment

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

This is a test class, cant it be changes right away?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because the real classes are using EntityManager in its argument declarations.
By instance:

Parameter #1 $em of method SimpleThings\EntityAudit\AuditManager::createAuditReader() expects Doctrine\ORM\EntityManager, Doctrine\ORM\EntityManagerInterface given.

VincentLanglet
VincentLanglet previously approved these changes Oct 27, 2021
@phansys
Copy link
Member Author

phansys commented Oct 27, 2021

@jordisala1991, @franmomu; could you please review again?

composer.json Outdated
"symfony/phpunit-bridge": "^5.3 || ^6.0"
"symfony/http-kernel": "^4.4 || ^5.3 || ^6.0",
"symfony/phpunit-bridge": "^5.3 || ^6.0",
"symfony/security-core": "^4.4 || ^5.3 || ^6.0",
Copy link
Member

Choose a reason for hiding this comment

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

this is already on require section

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Removed.

composer.json Outdated
"symfony/cache": "^4.4 || ^5.3 || ^6.0",
"symfony/http-foundation": "^4.4 || ^5.3 || ^6.0",
"symfony/phpunit-bridge": "^5.3 || ^6.0"
"symfony/http-kernel": "^4.4 || ^5.3 || ^6.0",
Copy link
Member

Choose a reason for hiding this comment

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

this one too

VincentLanglet
VincentLanglet previously approved these changes Oct 27, 2021
composer.json Outdated
"symfony/cache": "^4.4 || ^5.3 || ^6.0",
"symfony/http-foundation": "^4.4 || ^5.3 || ^6.0",
"symfony/phpunit-bridge": "^5.3 || ^6.0"
"symfony/phpunit-bridge": "^5.3 || ^6.0",
"symfony/var-dumper": "^4.4 || ^5.2",
Copy link
Member

Choose a reason for hiding this comment

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

you should use 4.4 5.3 and 6.0. To be consistent with the other symfony dependencies.

jordisala1991
jordisala1991 previously approved these changes Oct 28, 2021
VincentLanglet
VincentLanglet previously approved these changes Oct 28, 2021
@phansys phansys merged commit e22914a into sonata-project:1.x Oct 28, 2021
@phansys phansys deleted the phpstan branch October 28, 2021 13:58
@jordisala1991
Copy link
Member

Please remember to open the dev-kit PR to add psalm and phpstan @phansys

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.

6 participants