rtconner / laravel-tagging

Tag support for Laravel Eloquent models - Taggable Trait
MIT License
882 stars 168 forks source link

Add Auto Tagging from Property #90

Closed jakejohns closed 8 years ago

jakejohns commented 8 years ago

Allow setting a property (eg via a form field) to use as basis of retagging or untagging.

// with $autoTagFromProp = 'tag_names'
$model->tag_names = 'foo, bar';
$model->save();

I do something like this sometimes to be able to add the trait to a model and just add a field to a form to implement tagging. This allows me to just set a property of the model and automatically retag or untag it based on that property when just calling Model::save(), rather than calling retag or untag manually.

Thoughts? Concerns?

rtconner commented 8 years ago

I like the idea .. couldn't this have been done with a lot less code ..

    public function setTagNamesAttribute($value)
    {
        $this->retag($value);
    }

I don't see the need for making it a config option, since it a non-breaking change.

jakejohns commented 8 years ago

Ah. You might be right. However, I think that will fail if the entity has not already been persisted, no? i.e. if you set that attribute before that instance of model has been saved.

Model::create(['name'=>'foo', 'tag_names' => 'foo, bar, baz']);

I might be wrong, but I thought there was a reason I didn't do it that way.

jakejohns commented 8 years ago

Also, as I'm thinking about it, I don't think hitting the DB from a setter method is a good idea, as it seems a little too magical. model::save() is explicitly hitting the persistence layer. What if I want to do some mass assignment, and then some other type of validation or something, and NOT persist my changes?

rtconner commented 8 years ago

Nah, using ::create would call fill() which eventually calls ->setAttribute (which calls the magic attribute)

setTagNamesAttribute would work fine.

jakejohns commented 8 years ago

With model::setTagNamesAttribute:

 $ php artisan tinker
Psy Shell v0.6.1 (PHP 5.6.14-0+deb8u1 — cli) by Justin Hileman
>>> $x = new App\Foo
=> App\Foo {#683}
>>> $x->tag_names = 'foo, bar, baz';
Illuminate\Database\QueryException with message 'SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'taggable_id' cannot be null (SQL: insert into `tagging_tagged` (`tag_name`, `tag_slug`, `taggable_type`, `taggable_id`) values (Foo, foo, App\Foo, ))'

But if you call $model->retag() before $model->save(), then $model does not yet have an id and the retag call will fail.

rtconner commented 8 years ago

Interesting. That's a usage I hadn't ever considered.

What of the config? I still think that's overkill personally. Just always have it on.

I'm in the middle in messing around with trying to add unit tests, anyways so I can't merge anything this minute.

jakejohns commented 8 years ago

I'm fairly cetain Model::create() will throw the same exception as what I'm describing above.

RE: Config

Right on. I figured it might make sense to give the ability to disable it, or change the property name, but I suppose you can just overide the method or something if one wishes to do that.

I pushed some changes which remove the config and also changes a couple of other things.

The way it was, saving a model which did not have tag_names set would call untag. This is semi desired function, I think, as I would want untag called if I set tag_names to null or false or (empty string) or []. However, if one simply does not set tag_names at all, it probably shouldn't untag.

So I added a mutator which gives us the ability to see if it was explicitly set. If it's not it doesn't do anything in the post-save hook.

Note, the way this is, tag_names needs to be set to something truthy. i.e. you can't tag something 0, but I think this is actually inline with existing function.

re: unit tests

yay! ... no rush or anything.