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

[12.x] Add NamedScope attribute #54450

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

shaedrich
Copy link
Contributor

@shaedrich shaedrich commented Feb 3, 2025

Instead of

class User extends Model
{
    public scopeActive(Builder $builder)
    {
        // …
    }
}

you can now write

use Illuminate\Database\Eloquent\Attributes\NamedScope;

class User extends Model
{
    #[NamedScope]
    public active(Builder $builder)
    {
        // …
    }
}

similar to #40022

class User extends Model
{
    public getActiveAttribute()
    {
        // …
    }
}

and

class User extends Model
{
    public active()
    {
        return Attribute::make(
            get: fn() => // …
        );
    }
}

Complements #50034

FAQs

What if I already have a method that has the same name as a scope?

Your application will not be broken because the method does not have the NamedScope attribute, which did not exist before this pull request.

Will the old, multi-method approach of defining local scopes go away?

No. This is just an alternative approach.

@taylorotwell
Copy link
Member

Drafting pending for @crynobone - please mark as ready for review once reviewed.

@taylorotwell taylorotwell marked this pull request as draft February 5, 2025 10:41
@crynobone
Copy link
Member

@shaedrich using public method here means the method can be called directly and it would be common the pass the instance of Illuminate\Database\Eloquent\Builder. IDE with autocomplete would assume the first parameter to be Builder while we usually can pass something else.

@shaedrich
Copy link
Contributor Author

I can make it private/protected—no problem 👍🏻

return null;
}

$method = new ReflectionMethod($this, $scope);
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we need to check if $method return false for incorrect usage such as abstract method or public function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In lines 1638 – 1640, we test if the method exists—isn't that enough, meaning do we care about how the method is implemented?

@@ -27,4 +35,10 @@ public function scopeExists()
{
return true;
}

#[NamedScope]
public function existsAsWell()
Copy link
Member

Choose a reason for hiding this comment

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

Need to also test if child model can utilise attribute named scope defined in parent model.

method need to be updated to protected and contains parameter as example to real usage

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 afraid, I can't follow you? Can you elaborate on that?

Copy link
Member

@crynobone crynobone Feb 5, 2025

Choose a reason for hiding this comment

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

CleanShot 2025-02-05 at 19 48 00

This would be the actual usage, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct 👍🏻

Signed-off-by: Mior Muhammad Zaki <[email protected]>
Copy link
Member

@crynobone crynobone left a comment

Choose a reason for hiding this comment

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

Given the following example:

<?php

namespace App\Models;

// use Illuminate\Contracts\Auth\MustVerifyEmail;
use Illuminate\Contracts\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Attributes\NamedScoped;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Foundation\Auth\User as Authenticatable;
use Illuminate\Notifications\Notifiable;

class User extends Authenticatable
{
    /** @use HasFactory<\Database\Factories\UserFactory> */
    use HasFactory, Notifiable;

    /**
     * The attributes that are mass assignable.
     *
     * @var list<string>
     */
    protected $fillable = [
        'name',
        'email',
        'password',
    ];

    /**
     * The attributes that should be hidden for serialization.
     *
     * @var list<string>
     */
    protected $hidden = [
        'password',
        'remember_token',
    ];

    /**
     * Get the attributes that should be cast.
     *
     * @return array<string, string>
     */
    protected function casts(): array
    {
        return [
            'email_verified_at' => 'datetime',
            'password' => 'hashed',
        ];
    }

    #[NamedScoped]
    public function verified(Builder $builder, bool $email = true)
    {
        return $builder->when(
            $email === true,
            fn ($query) => $query->whereNotNull('email_verified_at'),
            fn ($query) => $queryu->whereNull('email_verified_at'),
        );
    }

    public function scopeVerifiedUser(Builder $builder, bool $email = true)
    {
        return $builder->when(
            $email === true,
            fn ($query) => $query->whereNotNull('email_verified_at'),
            fn ($query) => $queryu->whereNull('email_verified_at'),
        );
    }
}

Accessing named scope via query builder:

CleanShot 2025-02-06 at 18 33 13

Accessing named scope statically:

CleanShot 2025-02-06 at 18 34 17

Converting public function verified to protected function verified

CleanShot 2025-02-06 at 18 35 13

Signed-off-by: Mior Muhammad Zaki <[email protected]>
@crynobone
Copy link
Member

Added failing tests based on above finding

Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
@Bosphoramus
Copy link

Wouldnt #[LocalScope] be a better name for the attribute?

@antonkomarev
Copy link
Contributor

#[QueryBuilderScope]

@shaedrich
Copy link
Contributor Author

Wouldnt #[LocalScope] be a better name for the attribute?

#[QueryBuilderScope]

Yeah, I thought about this, too.

  • #[LocalScope]: While this may be the "correct" name to differentiate it from global scopes, this might sound like a variable scope and therefore might be confusing without further context.
  • #[QueryBuilderScope]: While this avoids the ambiguity described above, it, in turn, doesn't differentiate between local and global scopes

@taylorotwell @crynobone What do you think?

@shaedrich shaedrich marked this pull request as ready for review February 8, 2025 17: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.

5 participants