PHPStan level 6 with AuditReader not generic

[franmomu]

Subject

Alternative to #487 making AuditReader not generic.

Raising PHPStan level after this to level 7 shows 23 errors.

I am targeting this branch, because this is BC.

Changelog

### Added
- Specify iterable types
### Changed
- Make `AuditReader` not generic

[phansys]

Just a less greedy suggestion:

Suggested change

	- '#Test::(data|provide)[a-zA-Z0-9_]+\(\) return type has no value type specified in iterable type#'
	- '#^Method SimpleThings\\EntityAudit\\Tests\\[a-zA-Z0-9_\\]+Test::(data|provide)[a-zA-Z0-9_]+\(\) return type has no value type specified in iterable type [a-zA-Z0-9_]+\.$#'

[franmomu]

Glad to see you back @phansys!

[VincentLanglet]

Is there a lot of dataprovider to fix ?

[phansys]

IMO, we should declare the template before its uses.

[VincentLanglet]

Removing the template would create issues for the sonata compatibility since there is one in the SonataAdmin interface
https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Model/AuditReaderInterface.php#L19

I wonder if we shouldn't have both

/**
 * @phpstan-template T of object
 */
class AuditReader
{
    /**
     * @phpstan-template TT of T
     * @phpstan-param class-string<TT>
     * @phpstan-return TT
     */
    public function find($classname): object
    {
    }
}

This way we can define an AuditReader<Foo|Bar>, but then
if we use find(Foo::class), it will be understood as a Foo object and not only Foo|Bar.

WDYT ?

We should update https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Model/AuditReaderInterface.php phpdoc then and the same could be done for the ModelManager https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Model/ModelManagerInterface.php#L55

[franmomu]

Removing the template would create issues for the sonata compatibility since there is one in the SonataAdmin interface https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Model/AuditReaderInterface.php#L19

I wonder if we shouldn't have both

/**
 * @phpstan-template T of object
 */
class AuditReader
{
    /**
     * @phpstan-template TT of T
     * @phpstan-param class-string<TT>
     * @phpstan-return TT
     */
    public function find($classname): object
    {
    }
}

This way we can define an AuditReader<Foo|Bar>, but then if we use find(Foo::class), it will be understood as a Foo object and not only Foo|Bar.

WDYT ?

IMHO that seems overcomplicated.

I understand that AuditReader::find should be generic and maybe some other methods, but to me this class seems to work more like the EntityManager from doctrine/orm rather than a repository.

[VincentLanglet]

I understand that AuditReader::find should be generic and maybe some other methods, but to me this class seems to work more like the EntityManager from doctrine/orm rather than a repository.

For SonataAdmin, it's considered as a repository, and the AuditManager would be the EntityManager
https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Model/AuditManagerInterface.php#L42-L46
We can use different AuditReader for different classes.

But I think the generic part of the AuditReader of this project can be removed,
this shouldn't break the SonataDoctrineORM static analysis, with this code https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/4.x/src/Model/AuditReader.php#L41-L44

[jordisala1991]

isnt this a class-string?

[franmomu]

yep, all these parameters are defined below as @phpstan-param

[jordisala1991]

isnt this a class-string?

[jordisala1991]

Another class-string?

[jordisala1991]

And another one

[jordisala1991]

Thanks @franmomu