Adding scopes to collections

[Jud]

This creates behavior discussed in #115. Our use case is summed up in:

#115 (comment)

[lox]

Yeah, I think this is probably the most sane way to do this. Another option I considered was deferring the construction of a Collection instance to the DomainObject and then adding a way to register scopes with that instance:

<?php
class Payment extends DomainObject {
  public static function collection($query, $adder) {
    $col = new Collection(__CLASS__, $query, $adder);
    $col->scope('store_credit', function($chain){ 
       return $chain->filter('payment_type = ?', STORE_CREDIT_TYPE') 
    });
    return $col;
  }
}

That would let you easily change to using a custom Collection instance when the complexity of your scopes got out of control. I suspect your way is simpler.

[lox]

My only other gripe is that I think it would be clearer if scopes used method() syntax rather than attribute access:

<?php
$store_credits = $current_user->payments->store_credits();

Thoughts?

[Jud]

My thought re: method vs attribute was keeping consistency with relationships. Though I don't have a strong opinion either way.

[lox]

My only thinking was that it's slightly less magical as a method, then in the future if we support custom collections the calling syntax will stay the same in chains. It's probably a positive that it's visually obvious which is a relationship / attribute vs a scope.

[Jud]

@lox moved the logic into __call.

[lox]

This is looking good too, nice one @Jud. @harto thoughts?

[harto]

Should we throw a method-not-found exception in an else block here? Otherwise we'll be silently swallowing errors (or rather, returning null from missing methods).

[harto]

Looks good. Could use a note in the docs, perhaps.

[Jud]

@lox @harto -- that should do it

[harto]

Looks good. My only final suggestion would be to add a usage example of the active scope defined in the README.

[lox]

I'm going to save this for the 1.4 release which I am going to try and get our soon.

[Jud]

@harto @lox updated the readme to add invocation.

[Jud]

@lox would love to at least get this merged in. We've got lots of code we could simplify with scopes and would rather not have to vendor it.

[lox]

Yup, absolutely. Apologies for the delay.