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

First fix #92

Closed
wants to merge 5 commits into from
Closed

First fix #92

wants to merge 5 commits into from

Conversation

Orkin
Copy link
Member

@Orkin Orkin commented Sep 16, 2013

Use doctrineAdapter in case of using DoctrineORM to prevent this problem : #91

I'm not sure if this is what @bakura10 had in mind.

@Ocramius
Copy link
Member

@Orkin please: meaningful commit messages.

@Orkin
Copy link
Member Author

Orkin commented Sep 16, 2013

@Ocramius Sorry for my commit message, I was not inspired

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling b0c79f0 on Orkin:DoctrineOrmModule-paginator into 03ec0ea on zf-fr:master.

@bakura10
Copy link
Member

You can change your commit message by doing git commit --amend -m "Blabla"

@@ -250,7 +254,11 @@ protected function buildRouteMatch(ResourceInterface $resource, $path)
}

// @TODO: for now, collection is always wrapped around a ResourcePaginator, should instead be configurable
$data = new ResourcePaginator($resourceMetadata, new SelectableAdapter($data, $criteria));
if ($data instanceof EntityRepository && class_exists('DoctrineORMModule/Paginator/Adapter/DoctrinePaginator')) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure you should use the namespace separator for class exists.

@@ -250,7 +254,11 @@ protected function buildRouteMatch(ResourceInterface $resource, $path)
}

// @TODO: for now, collection is always wrapped around a ResourcePaginator, should instead be configurable
$data = new ResourcePaginator($resourceMetadata, new SelectableAdapter($data, $criteria));
if ($data instanceof EntityRepository && class_exists('DoctrineORMModule/Paginator/Adapter/DoctrinePaginator')) {
$data = new ResourcePaginator($resourceMetadata, new DoctrineAdapter(new DoctrinePaginator($data->find(array_shift($chunks)))));
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. In fact it's not that easy. https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Tools/Pagination/Paginator.php#L62

The consrtuctor of Doctrine paginator accepts either a Query or a Query builder. Hhmm... Let me think about it tomorrow.

@coveralls
Copy link

Coverage Status

Coverage decreased (-8.19%) when pulling fb14da8 on Orkin:DoctrineOrmModule-paginator into 03ec0ea on zf-fr:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling fb14da8 on Orkin:DoctrineOrmModule-paginator into 03ec0ea on zf-fr:master.

@@ -250,7 +254,11 @@ protected function buildRouteMatch(ResourceInterface $resource, $path)
}

// @TODO: for now, collection is always wrapped around a ResourcePaginator, should instead be configurable
$data = new ResourcePaginator($resourceMetadata, new SelectableAdapter($data, $criteria));
if ($data instanceof EntityRepository && class_exists('DoctrineORMModule\Paginator\Adapter\DoctrinePaginator')) {
$data = new ResourcePaginator($resourceMetadata, new DoctrineAdapter(new DoctrinePaginator($data->find(array_shift($chunks)))));
Copy link
Member

Choose a reason for hiding this comment

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

This does not handle pagination. The DoctrinePagination of the ORM module works with DQL queries

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I realized this. I'm going to close as the logic is broken.

Today I'm going to give a new try to fix that directly in Doctrine ORM.

@bakura10
Copy link
Member

As discussed with @Ocramius , the fix cannot work in fact, because Doctrine Paginator expects DQL, and we don't have this in ResourceMatch. So the simplest is to solve this directly at Doctrine level. I'll try to come with a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants