laravel-tags icon indicating copy to clipboard operation
laravel-tags copied to clipboard

Added support for strings and corresponding tests

Open zupolgec opened this issue 2 years ago • 2 comments

Scopes withAllTags and withAnyTags used to work with strings, it was just an issue with function argument typing. Added test cases.

At the same time I found out that attribute mutator setTagsAttribute was already accepting a tag as string and passing the argument to syncTags which, however, didn't support that. Fixed the test case of adding a single tag by mutator by removing the array brackets and making it fail, then solved by array wrapping the parameter.

zupolgec avatar Mar 17 '22 17:03 zupolgec

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

spatie-bot avatar Jul 18 '22 10:07 spatie-bot

@freekmurze I still feel this is useful. Can you please check? Thanks!

zupolgec avatar Jul 18 '22 10:07 zupolgec

Thanks!

freekmurze avatar Oct 21 '22 07:10 freekmurze

Hi, firstly, thanks guys for your improvements and work on this nice package! However, unfortunately, this PR seems to have broken the syncTags method when using arrays. When I try to sync Tags it creates strange arrays in the name field. Rolling back to 4.3.2 fixes this issue. Have any of you had seen this issue yet? Screenshot 2022-11-16 at 16-14-18@2x

janiskelemen avatar Nov 16 '22 15:11 janiskelemen

@zupolgec could you take a look?

freekmurze avatar Nov 16 '22 17:11 freekmurze

@janiskelemen I can't reproduce the issue. If a Model is Taggable, then you can use $model->syncTags(['tag1', 'tag2']) to substitute any tags the model already has with tag1 and tag2. That feature is working as intented.

zupolgec avatar Nov 16 '22 18:11 zupolgec

@zupolgec I think it happens when you pass a Tag collection into syncTags: $model->syncTags(Tag::take(3)->get())

janiskelemen avatar Nov 16 '22 18:11 janiskelemen

Thank you @janiskelemen!

@freekmurze fixed in #427. Sorry for introducing a bug!

zupolgec avatar Nov 17 '22 10:11 zupolgec