Have `DoctrinePaginator` implement `JsonSerializable` to improve cache key creation

doctrine / DoctrineORMModule

Doctrine ORM Module for Laminas

https://www.doctrine-project.org/projects/doctrine-orm-module.html

MIT License

439 stars 229 forks source link

Have `DoctrinePaginator` implement `JsonSerializable` to improve cache key creation #670

Closed FabianKoestring closed 3 years ago

FabianKoestring commented 3 years ago

fixes #669

Adds JsonSerializable to src/DoctrineORMModule/Paginator/Adapter/DoctrinePaginator.php to optimize cache-key generation in laminas/laminas-paginator _getCacheInternalId().

driehle commented 3 years ago

There are some issues regarding coding standards, could you please have a look at this?

Besides, we should at least have a unit test for the return value of jsonSerialize(). This can be done by mocking a paginator.

FabianKoestring commented 3 years ago

There are some issues regarding coding standards, could you please have a look at this?

Besides, we should at least have a unit test for the return value of jsonSerialize(). This can be done by mocking a paginator.

@driehle - I can add tests but why are the AdapterTest actually ignored.

driehle commented 3 years ago

@FabianKoestring Hm, I have no idea, but I was able to identify the commit that renamed AdapterTest.php to AdapterTestIgnore.php, which is db8532d250cec34a9aea4e6ed612baca8e3812e3 from 2012. So it looks like this test has been disabled for quite a long time.

Could you please try enabling that test and see if you can get it running?

Regarding the CS issue, I think PHPCS wants some more information about the type of array. You could use something generic like @return array<string, mixed>, but in this case it would be better to use @return array{select: string, count_select: int}, as will provide static code analysis much more information. You can run composer cs-check locally to see if all issues were fixed.

driehle commented 3 years ago

Ok, here are the changes required to make this test run again:

in test/DoctrineORMModule/Assets/Entity/FormEntity.php line 23 bool has to be changed to boolean
in test/DoctrineORMModule/Assets/Entity/Issue237.php line 9 doctrine_orm_module_form_entity has to be changed to doctrine_orm_module_form_entity_issue237
class DoctrineORMModuleTest\Paginator\AdapterTestIgnore should be renamed to DoctrineORMModuleTest\Paginator\AdapterTest

This gets it running for me. Try running composer test locally.

FabianKoestring commented 3 years ago

@driehle There are still some problems with php7.4.

driehle commented 3 years ago

@FabianKoestring You can see the output here, the failing job is PHPUnit (7.4, highest, false). The parameters in brackets come from the build matrix and stand for php-version=7.4, dependencies=highest and option_dependencies=false. The output is:

1) DoctrineORMModuleTest\Paginator\AdapterTest::testCanSetPaginator
Error: Class 'Doctrine\Common\DataFixtures\Loader' not found

/home/runner/work/DoctrineORMModule/DoctrineORMModule/test/DoctrineORMModuleTest/Paginator/AdapterTest.php:33

(repeated five times)

This is because in that job optional dependencies (doctrine/migrations and doctrine/data-fixtures) are removed prior to testing. I suggest you have a look here how to use $this->markTestAsIncomplete() when doctrine/data-fixtures is not available. You can simply check for the existence of Doctrine\Common\DataFixtures\Loader. You will likely need to do this as early as in setUp().

FabianKoestring commented 3 years ago

@driehle - Now everything should fit. Thank you for your support.

driehle commented 3 years ago

Indeed, now it's good! Your commits should be squashed though, it I'll take care of that prior to merging.

Thank you for your time and effort, @FabianKoestring!