DirectoryTree / LdapRecord

A fully-featured LDAP framework.
https://ldaprecord.com
MIT License
500 stars 44 forks source link

[Feature] `when` on `\LdapRecord\Query\Builder` #730

Closed joakimbergros closed 1 month ago

joakimbergros commented 1 month ago

Hey,

I'd like to see if adding the Illuminate\Support\Traits\Conditionable trait to the LdapRecord\Query\Builder class would be of interest?

I find myself liking the fluent syntax over saving the builder to a variable and then use conditionals to mutate the builder if necessary.


use LdapRecord\Models\ActiveDirectory\Group;

$builder = Group::query();

if (true) {
    $builder->where('name', '=', 'test');
}

$result = $builder->get();

vs


use LdapRecord\Models\ActiveDirectory\Group;
use LdapRecord\Query\Builder;

$result = Group::query()
    ->when(true, fn (Builder $builder) => $builder->where('name', '=', 'test'))
    ->get();

I'd love to hear your thoughts!

I tested this on my own fork and it works nicely, so a PR is ready if you decide it's something you'd want.

Cheers

joakimbergros commented 1 month ago

I just noticed it's available as a standalone dependency as well with illuminate/conditionable. It would follow the same structure as illuminate/collection and illuminate/contracts currently in the comsposer.json.

Edit: Seems the illuminate/collection already has the dependency.

stevebauman commented 1 month ago

Hey @joakimbergros!

I think adding the trait sounds good to me -- though unfortunately it doesn't have a v8 version which is the minimum we currently target in the composer.json:

https://github.com/DirectoryTree/LdapRecord/blob/2ccba8b7a3c2f316fcf07fa826515a43a9508db4/composer.json#L39-L40

Screenshot 2024-07-17 at 10 06 11 AM

We'll have to drop support for v8.0 versions, to be able to add this trait.

joakimbergros commented 1 month ago

I see, that's a bummer. Thanks for taking the time to check at the very least, appreciate it!

Decorator pattern to the rescue :)

stevebauman commented 1 month ago

No worries @joakimbergros! On the next major release I'll be removing support for these versions, so we can add the trait then 👍

If you'd like, you can add this trait directly onto the query builder by using your own model and then your own Builder instance (the same way you can do with Laravel's Eloquent):

namespace App\Ldap\Models;

use App\Ldap\Builders\UserBuilder;
use LdapRecord\Models\ActiveDirectory\User as BaseUser;

class User extends BaseUser
{
    /**
     * Create a new query builder.
     */
    public function newQueryBuilder(Connection $connection): UserBuilder
    {
        return new UserBuilder($connection);
    }
}
namespace App\Ldap\Builders;

use LdapRecord\Query\Model\Builder;
use Illuminate\Support\Traits\Conditionable;

class UserBuilder extends Builder
{
    use Conditionable;
}
User::query()->when('...');
joakimbergros commented 1 month ago

That's a great solution for now, thanks for the suggestion!

stevebauman commented 1 month ago

Happy to help! 🙏