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

[11.x] Add CollectedBy attribute #53122

Merged
merged 9 commits into from
Oct 15, 2024
Merged

[11.x] Add CollectedBy attribute #53122

merged 9 commits into from
Oct 15, 2024

Conversation

alsterholm
Copy link
Contributor

@alsterholm alsterholm commented Oct 11, 2024

This pull request adds a new CollectedBy attribute for specifying a custom Collection class for a model.

With this attribute added, one would only have to add the attribute rather than override the newCollection method on the Model class (or taking the undocumented route and setting the $collectionClass static property).

#[CollectedBy(PostCollection::class)]
class Post
{
    // ...
}

This also brings the approach closer in line with the new ObservedBy and ScopedBy attributes.

@taylorotwell
Copy link
Member

taylorotwell commented Oct 12, 2024

May be worth memoizing the resolution from the attribute? Otherwise it's a lot of reflection operations each time this method is called.

@alsterholm
Copy link
Contributor Author

May be worth memoizing the resolution from the attribute? Otherwise it's a lot of reflection operations each time this method is called.

Very good point! I've updated the PR and wrapped the logic in the resolveCollectionAttribute method in a once call.

Co-authored-by: Jeffrey Angenent <1571879+devfrey@users.noreply.github.com>
@@ -15,6 +18,27 @@ trait HasCollection
*/
public function newCollection(array $models = [])
{
return new static::$collectionClass($models);
$collectionClass = $this->resolveCollectionAttribute() ?? static::$collectionClass;
Copy link
Contributor

@johanrosenson johanrosenson Oct 14, 2024

Choose a reason for hiding this comment

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

instead of once() this could also work, IMO easier to debug and troubleshoot.

static::$resolvedCollectionClass ??= $this->resolveCollectionAttribute() ?? static::$collectionClass;

return new static::$collectionClass($models);

required addition to Model.php

protected static $resolvedCollectionClass = null;

@taylorotwell
Copy link
Member

I believe once may not be the best answer here because it is scoped to a particular object instance, whereas you probably want a truly static variable like @johanrosenson mentions.

/**
* Create a new attribute instance.
*
* @param class-string $collectionClass
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably match the typehint in Database\Eloquent\Model#L228

@param class-string<\Illuminate\Database\Eloquent\Collection<*, *>> $collectionClass

@alsterholm
Copy link
Contributor Author

alsterholm commented Oct 14, 2024

@johanrosenson @taylorotwell But using a static variable like that wouldn't work as the resolved value would be shared across all models:

abstract class Model
{
    public static $resolvedCollectionClass = null;
}

class A extends Model
{
    public function resolveCollectionClass()
    {
        static::$resolvedCollectionClass = CustomCollection::class;
    }
}

class B extends Model {}

(new A)->resolveCollectionClass();

var_dump(B::$resolvedCollectionClass); // string(16) "CustomCollection"

I suppose an array in a static variable used as a lookup table, where the key is the model class name, could work?

/**
* @template TCollection of \Illuminate\Database\Eloquent\Collection
*/
trait HasCollection
{
/**
* @var array<class-string<static>, class-string<TCollection>>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure about this type. If a phpstan wizard would like to have a look at this, that would be awesome ☺️

@browner12
Copy link
Contributor

Is this better than overriding the newCollection method, or just different?

@shaedrich
Copy link
Contributor

@browner12 From a DX standpoint, it is primarily different; from a semantic standpoint, from a semantic(?) it gravitates more towards being declarative while newCollection() would be more imperative

@taylorotwell taylorotwell merged commit 04e2e19 into laravel:11.x Oct 15, 2024
31 checks passed
@alsterholm alsterholm deleted the collected-by-attribute branch October 15, 2024 14:38
@alsterholm
Copy link
Contributor Author

@taylorotwell Do you want me to submit a PR to the docs documenting the attribute?

@shaedrich
Copy link
Contributor

@alsterholm fyi, you might be interested in this: #53126

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.

6 participants