Problem when overriding the Tag Model

[ojsoft]

In short:

protected static function convertToTags from HasTags.php

uses

if ($value instanceof Tag)

Well I use my own Tag Model and this test does not pass... so when I have Tag instances and pass them to the withAnyTag Scope for example, they are not recognized and therefore ...

Please change this place to:

if ($value instanceof $className)

Full code to replace for your convinience:

protected static function convertToTags($values, $type = null, $locale = null)
    {
        return collect($values)->map(function ($value) use ($type, $locale) {
            $className = static::getTagClassName();
            if ($value instanceof $className) {
                if (isset($type) && $value->type != $type) {
                    throw new InvalidArgumentException("Type was set to {$type} but tag is of type {$value->type}");
                }

                return $value;
            }

            return $className::findFromString($value, $type, $locale);
        });
    }

[freekmurze]

You can solve this by extending the tag model provided by this package.

[ojsoft]

Hello,

Well actually I would consider this a bug in your code and it is not in the Tag Model, its in the HasTags trait. The problem cannot be solved by overriding the Tag Model. The problem appears due to the fact I overrode the Tag Model with my own model.

Of course I could override the whole function in all Models that use Tagging with another trait and a.s.o. but this is only a bad workaround.

Your code from the HasTags trait: protected static function convertToTags($values, $type = null, $locale = null) { return collect($values)->map(function ($value) use ($type, $locale) { if ($value instanceof Tag) { if (isset($type) && $value->type != $type) { throw new InvalidArgumentException("Type was set to {$type} but tag is of type {$value->type}"); }

        return $value;
    }

    $className = static::getTagClassName();

    return $className::findFromString($value, $type, $locale);
});

}

see if you are allowing me to hand you my own Tag Model, you should honor this in your trait, you do at a lot of other places…

This way would be the better way, wouldn’t it or do i miss a point? My suggestion with the „fix": $className = static::getTagClassName(); if ($value instanceof $className) { if (isset($type) && $value->type !== $type) { throw new InvalidArgumentException("Type was set to {$type} but tag is of type {$value->type}"); }

return $value;

}

With best regards,

Oliver Jörns

Am 14.10.2017 um 12:05 schrieb Freek Van der Herten notifications@github.com:

You can solve this by extending the tag model provided by this package.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spatie/laravel-tags/issues/69#issuecomment-336624915, or mute the thread https://github.com/notifications/unsubscribe-auth/AUFPK1LFgOToAOjBrr94xXQ_C3IgZAkeks5ssId8gaJpZM4P4SXs.

[freekmurze]

I don’t understand the problem. Could you PR a failing test and a fix for me to look at?

[ojsoft]

Ok, I explain more deeply.

I have figured out a couple of Tags that I want to look for. The mechanism to find these Tags is a bit queer, because we have a multiinual site that does look into many languages to find the tags, therefore it makes no sense to put all possible writings into the query, but since the withAnyTag scope accepts Tag elements and not just tag strings I want to pass my Tags to the query.

class MyOwnTag extends Spatie\Tags\Tag

$tags_to_find = MyOwnTag::where(?)->get();

$myModel->withAnyTags($tags_to_find);

This would work fine if I would use your Tag Model, but I use my own in providing you the Tag Model like this in My Model that uses the HasTags Trait:

/**

• @return string */ public static function getTagClassName(): string { return MyOwnTag::class; }

Your scopeWithAnyTags function calls

$tags = static::convertToTags($tags, $type); And this function then calls the function findFromString($value, $type, $locale) because it does not recognize the MyOwnTag Model as a Tag model.

Since findFromString only accepts a string… but then gets my MyOwnTag collection this failes of course with an exception.

If convertToTags would recognise the replacement of the Tag Model to MyOwnTag Model, and just return them like it does with the original everything would work just fine.

Clear?

With best regards,

sorry for bothering you, I appreciate a lot the work you have done and that I am able to use that.

I am currently switching the Tagging in our software to yours, after the Change to Laravel 5.5.

Am 15.10.2017 um 20:05 schrieb Freek Van der Herten notifications@github.com:

I don’t understand the problem. Could you PR a failing test and a fix for me to look at?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spatie/laravel-tags/issues/69#issuecomment-336729606, or mute the thread https://github.com/notifications/unsubscribe-auth/AUFPK7OoH5M-v0pgaFjnUa835tRagD11ks5ssklhgaJpZM4P4SXs.

[freekmurze]

I'm thinking the scopes and function should work correctly even when using a custom model.

Maybe I still don't understand the problem... Please submit a failing test (and a maybe a fix) for me to look at.