Skip to content

[11.x] Add QueriedBy attribute #53176

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

Closed
wants to merge 1 commit into from

Conversation

alsterholm
Copy link
Contributor

@alsterholm alsterholm commented Oct 15, 2024

Along the same vein as the recently added CollectedBy attribute (#53122), I propose to add the QueriedBy attribute which allows users to configure which Builder class to use without having to override the newEloquentBuilder method.

#[QueriedBy(PostBuilder::class)]
class Post extends Model
{
    // ...
}

The implementation is basically a carbon copy of the CollectedBy attribute, with memoizing via a static array variable on the Model class.

@taylorotwell
Copy link
Member

Let me sit on this one a bit. I feel like custom query builders are much, much more rare than custom collections, which are also rare I think. 😅

@princejohnsantillan
Copy link
Contributor

I really hope this gets re-opened. I would love to see this in. I use custom eloquent builders in every project.

@rodrigopedra
Copy link
Contributor

@taylorotwell at least for those of us who follow @timacdonald for enough time, I guess dedicated query builders are way more common than custom collections, based on this post

https://tim.macdonald.au/dedicated-eloquent-model-query-builders

I do use 3 custom collections, on a single project. Mostly because there are lots of aggregated reports with different filters on the same view, that benefit from custom filters on those collections.

But since that post came out, in 2019, all of my projects use dedicated query builders for almost every single model.

@calebdw
Copy link
Contributor

calebdw commented Oct 17, 2024

I also wanted to chime in with my experience. I've never made a custom Collection while I heavily use custom Builders---the primary reason is for better static analysis / IDE autocompletion and to slim down the models by moving the scopes to the builders.

Larastan does support both custom Collections and Builders, but there's a lot more tests, logic, and discussions centered around the custom Builders

Comment on lines +131 to +139
$reflectionClass = new ReflectionClass(static::class);

$attributes = $reflectionClass->getAttributes(QueriedBy::class);

if (! isset($attributes[0]) || ! isset($attributes[0]->getArguments()[0])) {
return;
}

return $attributes[0]->getArguments()[0];
Copy link
Contributor

@calebdw calebdw Oct 17, 2024

Choose a reason for hiding this comment

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

Instead of looking at the arguments (and checking if they are set), you can just directly get the attribute instance and access the property:

$attribute = (new ReflectionClass(static::class))->getAttributes(QueriedBy::class)[0] ?? null;

return $attribute?->newInstance()->builderClass;

@pelmered
Copy link
Contributor

I would also use this a lot more than CollectedBy for the reasons stated in the comments before.

So, I hope you can reconsider this Taylor.

@vladislavs-poznaks
Copy link

Stumbled upon this pull request as with the recent addition of CollectedBy immediately thought about an attribute for a custom query as I tend to use custom query builders more than custom collections.

Fingers crossed on this to be reopened.

@MannikJ
Copy link
Contributor

MannikJ commented Oct 24, 2024

As seen most people discussing here are aware of the pros, but I just wanted to add my two cents as well to emphasize that adding custom query builders is more than some neat sugar feature with little value that nobody uses.

I have been using custom query builders for years now as well as custom collections, but if I had to decide which one of them to choose I would go for the builders all the time.
Custom builders are very powerful and make the models a lot cleaner especially if you are using a lot of query scopes and provides better support for static analysis and typing as well.
It reduces the model classes to almost only attributes and actions, without changing their actual interface in any way. This is a no-brainer for me and should be used a lot more than it obviously is.

That being said, I wouldn't actually need the attribute for this, it would just be the logical step since CollectedBy is already there.
While I understand the reasons given against integrating this attribute, I personally find them less convincing, especially considering the benefits custom query builders bring.
We should be moving people in the direction of using custom builders rather than portraying it as something very special - and the attribute could help with that.

@pelmered
Copy link
Contributor

Yes, it just doesn't make sense to include attributes like ScopedBy, CollectedBy and ObeservedBy, but omit this one. Out of all these four, QueriedBy would be the one I would use the most.

What is the main arguments against adding this? The maintenance burden? Performance? Wouldn't that apply to the three others as well?

@adamthehutt
Copy link
Contributor

At the risk of piling on... I use tons of custom query builders and no custom collections. I would love having this attribute in the core framework. Hoping you'll reconsider @taylorotwell.

@johanrosenson
Copy link
Contributor

johanrosenson commented Nov 19, 2024

My understanding is that Taylor usually doesn't look at closed PR's, so I think someone needs to send in a new PR and summarize everything that have been added here since it's closing.

If you want him to take another look that is.

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.

10 participants