cviebrock/eloquent-taggable

The tag model doesn't prevent duplicates

philippejadin opened this issue · 7 comments

philippejadin commented

As far as I can tell, I can create multiple tags using the tag model with the same normalized name. It would be nice to disallow this at the db level (in the migration?) or at least on the model when it is saved.

Can be done? should be done?

Thanks you!

judgej commented

I'm looking at the database structure and there are a number of issues that should be fixed to maintain data integrity:

  • Use of big integers for all keys. This is a Laravel 5.8 standard out of the box, but TBH most of us should be using bit ints anyway.
  • There is an index on taggable_taggables.taggable_type. The cardinality of this table is normally going to be tiny (maybe just one or two), so an index will be less help than none at all.
  • A unique index on taggable_taggables covering tag_id, taggable_id and taggable_type will ensure no model gets the same tag more than once. A non-unique index already covers the last two columns, so the taggable_taggables can be added to the end of that without affecting its current indexing use.
  • There is no referential integrity constraints between the taggable_taggables and taggable_tags tables. This should be strictly enforced to help avoid unexpected problems when links become invalid. The relationship could be set to cascade, so when a tag is deleted, its use across all the models would go too.

Just allowing the migration file to be published would be a great first move here, then we can add and modify the indexes we like.

On a side note, the naming conventions for the primary key and foreign keys don't follow Laravel conventions, and this means workarounds and more time needed to understand what the database structure is.

Apart from that - it's the best tagging plugin by far :-)

mateusjunges commented

@philippejadin @judgej @cviebrock Are you working on this?
I have some free time and if you agree, I can send a PR fixing this issue

judgej commented

I've not got any spare time for the next month at least. I'll fork this package if any of this becomes a blocker, then see what can be worked back in later.

mateusjunges commented

@judgej I'll work on that. I will be happy if you got some time to review my PR when it is ready.

  • 👍2
mateusjunges commented

@judgej I just opened the PR for this issue. You can check if you want.

  • 👍1
judgej commented

That's a great load of fixes. I'll aim to give it a try later today.

  • ❤️2
cviebrock commented

Version 6.0.2 should solve most of these issues.