Add an error message for non existing repository method & suggest to use CreatePaginatorTrait#1063
Conversation
c81a20a to
6ebfa9a
Compare
68bc7fc to
d49e43c
Compare
| protected ?array $validationContext = null, | ||
| private ?array $normalizationContext = null, | ||
| private ?array $denormalizationContext = null, | ||
| private ?array $validationContext = null, |
There was a problem hiding this comment.
already fixed on 1.14 branch.
b2d64c4 to
bdc48c0
Compare
bdc48c0 to
ede2d8c
Compare
e16a725 to
3689132
Compare
| /** | ||
| * @param array $objects | ||
| */ | ||
| protected function getArrayPaginator($objects): Pagerfanta |
There was a problem hiding this comment.
to avoid bc-breaks, I keep the missing argument type.
| </thead> | ||
| <tbody> | ||
| {% for resource in resources.data %} | ||
| {% if not definition %} |
There was a problem hiding this comment.
Sth that we could do on BootstrapAdminUi index template.
| if (!class_exists(QueryAdapter::class)) { | ||
| throw new \LogicException('You can not use the "paginator" if Pargefanta Doctrine ORM Adapter is not available. Try running "composer require pagerfanta/doctrine-orm-adapter".'); | ||
| } |
There was a problem hiding this comment.
Maybe move it to the top of ::createPaginator?
No point in applying criteria and sorting when it'll fail either way 🤷
There was a problem hiding this comment.
that's right, but maybe a user can call the getPaginator without use createPaginator method, cause this method is protected.
| /** | ||
| * @method ClassMetadata getClassMetadata() | ||
| * @method QueryBuilder createQueryBuilder(string $alias, string $indexBy = null) | ||
| */ | ||
| trait CreatePaginatorTrait |
There was a problem hiding this comment.
How about @mixin Doctrine\ORM\EntityRepository?
3689132 to
37ae304
Compare
| use Doctrine\ORM\QueryBuilder; | ||
| use Pagerfanta\Adapter\ArrayAdapter; | ||
| use Pagerfanta\Doctrine\ORM\QueryAdapter; | ||
| use Pagerfanta\Pagerfanta; |
There was a problem hiding this comment.
Are we ready for
Pagerfanta\PagerfantaInterfaceyet or it should be kept as it for BC?
There was a problem hiding this comment.
It seems to be ok so, let's go.
https://3v4l.org/O58qm
|
|
||
| <service id="sylius.registry.resource_repository" class="Sylius\Component\Registry\ServiceRegistry" public="false"> | ||
| <argument>Sylius\Component\Resource\Repository\RepositoryInterface</argument> | ||
| <argument>Doctrine\Persistence\ObjectRepository</argument> |
There was a problem hiding this comment.
This feels like a bigger change, is there some explanation why sylius/resource goes away from RepositoryInterface?
What will happen, if my custom repository doesn't implement any resource trait and interface?
There was a problem hiding this comment.
yes, the reason is that we do not need to implement that specific interface anymore in the new routing system.
This is the reason why we have this issue (cf first error message on first screenshot).
This RepositoryInterface from Sylius extends that ObjectRepository from Doctrine.
But we do not need these three specific methods required by the Sylius one anymore (add, remove, createPaginator).
| $repositoryInstance = $this->locator->get($repository); | ||
|
|
||
| if ( | ||
| !str_starts_with($method, 'find') && // magic calls on Doctrine repositories |
There was a problem hiding this comment.
// magic calls on Doctrine repositories
That's not always true, I can have methods like findOneByAcme, which doesn't have any magic.
There was a problem hiding this comment.
That's right, and not every "find" methods are magic.
We can replace the text with "To allow magic calls on Doctrine repository methods".
a0607da to
dba5772
Compare
dba5772 to
5f3251d
Compare
There was a problem hiding this comment.
We should probably get rid of psalm in favour of phpstan, as we did in other repositories 🤔
When using the "make:entity", a repository is created automatically, and normally it can be used directly with the new routing system without implementing the Sylius repository interface.
But here, in a collection operation, we define this createPaginator by default.
So here, first, the idea is to be more precise on what's happening when the default method and suggest using CreatePaginatorTrait.
That's another point, but we should also warn (or just fix it) about a non-grid object on the BootstrapAdminUi grid template.
Before

After
