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

Dont wrap in paginator #111

Merged
merged 5 commits into from
Dec 28, 2013
Merged

Dont wrap in paginator #111

merged 5 commits into from
Dec 28, 2013

Conversation

bakura10
Copy link
Member

ping @danizord @Ocramius

This PR removes the automatic conversion to Paginator in the router. Instead, the controller now receives exactly what has been matched by the router, so it's up to the user to specify the appropriate type hint (Selectable will be the most useful one I think). Query params filtering can be done directly in the action controller now, this is much more convenient.

Also, the CreateResourceModelListener correctly take into account the return value from the controller. So if you receive a collection, and manually build a paginator from it, the listener will detect the data is not the same (initially a collection, then a paginator), and will therefore create a new resource (reusing the same metadata).

I thing: should we include a utility method in AbstractRestfulController to create a paginator from a collection or is out from our responsibility?

@Ocramius
Copy link
Member

@bakura10 👍

I thing: should we include a utility method in AbstractRestfulController to create a paginator from a collection or is out from our responsibility?

Controller helpers to the rescue?

@coveralls
Copy link

Coverage Status

Coverage increased (+2.32%) when pulling 47cb8d0 on collection into 188c450 on master.

@bakura10
Copy link
Member Author

Good idea for the controller plugin! I'll do that later :). I've also updated the doc.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.32%) when pulling c756111 on collection into 188c450 on master.

@bakura10
Copy link
Member Author

Done @Ocramius !

I've added a paragraph on cookbook to show usage. Looks good to you?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.37%) when pulling 0a08b0c on collection into 188c450 on master.

*/
public function __invoke(Traversable $data, $criteria = [])
{
$paginatorAdapter = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

if ($data instanceof Selectable) {
    return new SelectableAdapter($data, $this->createCriteria($criteria));
}

if ($data instanceof Collection) {
    return new CollectionAdapter($data);
}

// throw exception

* @return Paginator
* @throws RuntimeException
*/
public function __invoke(Traversable $data, $criteria = [])
Copy link
Contributor

Choose a reason for hiding this comment

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

You may accept $criteria = null and call createCriteria() only if it is an array.

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 it should work with Criteria also. Let's keep it simple :).

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is null or Criteria you can pass directly to SelectableAdapter without calling createCriteria()

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d778526 on collection into 188c450 on master.

@@ -65,7 +65,13 @@ public function createResourceModel(MvcEvent $event)
return;
}

$model = new ResourceModel($resource);
// Because we may manipulate the resource data in the controller (for instance wrapping a collection around
// a paginator), we need to create a new resource from the data
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not create a method setData()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I had this discussion with @Ocramius and he told me we should not be able to change the resource.

Furthermore you may want to keep the matched resource untouched.

Envoyé de mon iPhone

Le 24 déc. 2013 à 12:11, Daniel Gimenes notifications@github.com a écrit :

In src/ZfrRest/Mvc/CreateResourceModelListener.php:

@@ -65,7 +65,13 @@ public function createResourceModel(MvcEvent $event)
return;
}

  •    $model = new ResourceModel($resource);
    
  •    // Because we may manipulate the resource data in the controller (for instance wrapping a collection around
    
  •    // a paginator), we need to create a new resource from the data
    
    Why not create a method setData()?


Reply to this email directly or view it on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

@danizord yep. Don't change the resource :-)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 18690e0 on collection into 188c450 on master.

* @return Paginator
* @throws RuntimeException
*/
public function __invoke($data, $criteria = [])
Copy link
Contributor

Choose a reason for hiding this comment

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

If $criteria is a Criteria or null you may pass it directly to the adapter without calling createCriteria()

public function __invoke($data, $criteria = null)
{
    if ($data instanceof Selectable) {
        if (is_array($criteria)) {
            $criteria = $this->createCriteria($criteria);
        }

        // Otherwise, pass $criteria directly to SelectableAdapter        
        return new Paginator(new SelectableAdapter($data, $criteria);
    }
    //...

@danizord
Copy link
Contributor

👍

bakura10 added a commit that referenced this pull request Dec 28, 2013
@bakura10 bakura10 merged commit 082814e into master Dec 28, 2013
@bakura10 bakura10 deleted the collection branch December 28, 2013 18:54
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.

4 participants