Description
A quick look into the way tags are added and removed proves to be very inefficient
public function tag($tags): bool
{
foreach ($this->prepareTags($tags) as $tag) {
$this->addTag($tag);
}
return true;
}
/**
* {@inheritdoc}
*/
public function addTag(string $name): void
{
$tag = $this->createTagsModel()->firstOrNew([
'slug' => $this->generateTagSlug($name),
'namespace' => $this->getEntityClassName(),
]);
if (! $tag->exists) {
$tag->name = $name;
$tag->save();
}
if (! $this->tags()->get()->contains($tag->id)) {
$tag->update(['count' => $tag->count + 1]);
$this->tags()->attach($tag);
}
$this->load('tags');
}
/**
* {@inheritdoc}
*/
public function removeTag(string $name): void
{
$slug = $this->generateTagSlug($name);
$namespace = $this->getEntityClassName();
$tag = $this
->createTagsModel()
->whereNamespace($namespace)
->where(function ($query) use ($name, $slug) {
$query
->orWhere('name', '=', $name)
->orWhere('slug', '=', $slug)
;
})
->first()
;
if ($tag && $this->tags()->get()->contains($tag->id)) {
$tag->update(['count' => $tag->count - 1]);
$this->tags()->detach($tag);
}
$this->load('tags');
}
As we can see we have a loop in which a lot of queries are produced $this->tags()->get()
and $this->load('tags')
are called which load all of the tags of the element twice (which can be a real problem if there is a lot of tags) in addition to the creation and count update
This can be upwards of 3 update and 3 select query per addition of one tag (so if you add 5 tags, you have 30 queries)
I think it shouldn't be the job of addTag to load the relation (especially twice on each call), so I would remove $this->load('tags');
and refactor the rest to be more efficient while still solving #26
public function addTag(string $name): void
{
$slug = $this->generateTagSlug($name);
$namespace = $this->getEntityClassName();
$tag = $this->createTagsModel()->firstOrCreate([
'slug' => $this->generateTagSlug($name),
'namespace' => $this->getEntityClassName(),
], [
'count' => 1,
'name' => $name,
]);
if ($tag->wasRecentlyCreated || !$this->tags()->exists($tag)) {
if (!$tag->wasRecentlyCreated) {
$tag->update(['count' => $tag->count + 1]);
}
$this->tags()->attach($tag);
if ($this->relationLoaded('tags')) {
$this->tags->add($tag);
}
}
}
This would result in only 2 update and 2 select queries in the worst case and 2 insert and 1 select in the best case
Same goes for the removeTag
method with a best case of just 1 select and a worst case of 1 select and 2 update
public function removeTag(string $name): void
{
$slug = $this->generateTagSlug($name);
$namespace = $this->getEntityClassName();
$tag = $this->tags()
->whereNamespace($namespace)
->where(function ($query) use ($name, $slug) {
$query
->orWhere('name', '=', $name)
->orWhere('slug', '=', $slug)
;
})
->first()
;
if ($tag->exists) {
$tag->update(['count' => $tag->count - 1]);
$this->tags()->detach($tag);
if ($this->relationLoaded('tags')) {
foreach($this->tags as $key => $loadedTag) {
if ($loadedTag->getKey() === $tag->getKey()) {
unset($this->tags[$key]);
break;
}
}
}
}
}
This could be even further improved by querying all tags at once outside of the loop, or using laravel's sync
method and diffing the tags before and after to update the counts