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 pending attributes #53720

Open
wants to merge 7 commits into
base: 11.x
Choose a base branch
from
Open

Conversation

tontonsb
Copy link
Contributor

Provides a withAttributes() method for Eloquent Builder. Doing ->withAttributes(['key' => 'value']) will instruct the Builder instance that the specified attributes must be added to new model instances if you create a model and used as where conditions if you end up doing a select.

Solves the same problem as #53677 (and satisfies its tests), but also provides withAttributes on the base builder.

Details

Consider this setup:

class User extends Model
{
    public function posts(): HasMany
    {
        $this->hasMany(Post::class);
    }

    public function hiddenPosts(): HasMany
    {
        $this->posts()->hidden();
    }
}

class Post extends Model
{
    public function scopeHidden(Builder $posts)
    {
        $posts->where('hidden', 'y');
    }
}

Currently Laravel enables features like:

$user->posts()->get();
$user->hiddenPosts()->get();
$user->posts()->create();

Post::all();
Post::hidden()->get();
Post::create();

However, syntax like $user->activePosts()->create() or Post::active()->create() will create a post without setting the value of active. Which means that post can be created by $user->activePosts()->create() but $user->activePosts()->get() will not select it.

This PR adds a withAttributes method that allows you to either

// in the relationship on the User model
public function hiddenPosts(): HasMany
{
    return $this->posts()->withAttributes('hidden', 'y');
}

or

// in the scope on the Post model
public function scopeHidden(Builder $posts)
{
    $posts->withAttributes('hidden', 'y');
}

And now the $user->hiddenPosts()->create() will create a Post with the hidden attribute set to 'y' and the Post will be found by $user->hiddenPosts()->get().

@taylorotwell
Copy link
Member

@tontonsb what do you think the behavior should be for many to many relationships?

@tontonsb
Copy link
Contributor Author

tontonsb commented Dec 1, 2024

Sorry @taylorotwell, I did not realize that the create() can also be done on a BelongsToMany.

I think it would be natural to expect that a relationship like this

    public function metaTags(): BelongsToMany
    {
        return $this->tags()
            ->withAttributes('visible', true)
            ->withPivotValue('type', 'meta');
    }

will select and create a Tag with visible set on the Tag and type set on the pivot table. I've implemented that and added a test for this case.

I'm not 100% sure whether the querying should auto-qualify the attributes' columns for wheres. A plain ->where('visible', true) doesn't do that so I'd keep ->withAttributes('visible', true) unqualified for consistency and let the developer choose.

On the other hand it seems that wrapping $column in $this->qualifyColumn($column) couldn't harm anything, could it? But in that case it might as well be done in the where() itself.

@taylorotwell
Copy link
Member

taylorotwell commented Dec 2, 2024

@tontonsb I do think that withAttributes should probably qualify on the where it generates, but I wouldn't change plain where behavior in this PR. I feel like where is a more bare-metal method and I wouldn't expect any auto-qualifying magic.

Does this PR work with morphable relationships?

@tontonsb
Copy link
Contributor Author

tontonsb commented Dec 2, 2024

Does this PR work with morphable relationships?

It does, I expanded the ManyToMany test with checks for morphToMany and morphedByMany. The morphMany and morphOne relationships already had tests.

I do think that withAttributes should probably qualify on the where it generates

Done.

@taylorotwell thanks for the guidance so far and let me know if anything else needs polishing here.

@MahdiSaremi
Copy link

Does withAttributes work with normal queries?

User::query()->withAttributes('active', true)->create();

I don't think it makes sense.
It's better to move the withAttributes method into the BelongsTo, BelongsToMany, HasMany and other relationship classes.

I feel like the name withAttributes doesn't really fit what it does. It feels like I'm going to add attributes to my model at the end, but behind the scenes it's adding a where condition!
Of course, this is my personal opinion and I have no idea for a more suitable alternative name :)

@tontonsb
Copy link
Contributor Author

Yes @MahdiSaremi it does work with Eloquent's base builder. I can agree that in your example this feature is not very useful, but to me it would be useful in scopes as it allows creating a scope for both creation and selection. E.g. scopes that function in both Post::hidden()->get() and Post::hidden()->create().

I feel like the name withAttributes doesn't really fit what it does. It feels like I'm going to add attributes to my model at the end, but behind the scenes it's adding a where condition!

I'm not saying this is the best name ever, but it makes sense to me if I think of ->withAttributes('active', true) as "from here on we are considering the active users". E.g. ->withAttributes('active', true)->get() will get you the entries with the 'active' attribute being true.

Part of the reason to add the where conditions is that ->withPivotValue() also works like that. You can see more on #53677 where the feature was inside the relationships as you propose.

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.

3 participants