Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Add support for getting Model morphed alias #38849

Closed
wants to merge 1 commit into from

Conversation

micc83
Copy link
Contributor

@micc83 micc83 commented Sep 17, 2021

Rationale

Allows to get the the morphed alias from the model class name. This can be useful in many scenarios where one wants to manually query the database (or any other source) with the given alias. One for all is described in the example below:

use Illuminate\Database\Eloquent\Relations\Relation;

Relation::morphMap([
    'children.a' => ChildrenA::class,
    'children.b' => ChildrenB::class,
]);

/** @property string $type */
abstract class Parent extends Model {
    protected $table = 'parent_table';
    public static function boot()
    {
        parent::boot();

        static::addGlobalScope(
            fn (Builder $query) => $query->where('type', Relation::getMorphedAlias(static::class))
        );
    }
}

class ChildrenA extends Parent {
}

class ChildrenB extends Parent {
}

Considerations

It's in no way breaking existing features as it simply adds a new static method to the Relation class.

As return value for getMorphedAlias I picked false|int|string as the key can be int|string. About the false value, we can replace it with null for consistency with getMorphedModel.

@binaryk
Copy link
Contributor

binaryk commented Sep 17, 2021

We had to use (new User)->getMorphClass() to get that.

@micc83
Copy link
Contributor Author

micc83 commented Sep 17, 2021

We had to use (new User)->getMorphClass() to get that.

Which in this case doesn't make much sense as it's much more expensive that doing a simple array_search on the $morphMap.

@inxilpro
Copy link
Contributor

Because Eloquent query builders always have an instantiated copy of the model they're associated with attached to them, you can currently do:

public static function booted()
{
  static::addGlobalScope(
    fn(Builder $query) => $query->where('type', $query->getModel()->getMorphClass())
  );
}

That said, if this PR were to be merged, you probably want to use the behavior in HasRelationships::getMorphClass and then update that method to call the new getMorphedAlias method.

Comment on lines +468 to +477
/**
* Get the alias associated with a given Model class name.
*
* @param string $model
* @return false|int|string
*/
public static function getMorphedAlias($model)
{
return array_search($model, static::$morphMap);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do something like this:

Suggested change
/**
* Get the alias associated with a given Model class name.
*
* @param string $model
* @return false|int|string
*/
public static function getMorphedAlias($model)
{
return array_search($model, static::$morphMap);
}
/**
* Get the alias associated with a given Model class name.
*
* @param string|\Illuminate\Database\Eloquent\Model $model
* @return string
*/
public static function getMorphedAlias($model)
{
if (! is_string($model)) {
$model = get_class($model);
}
$morphMap = Relation::morphMap();
if (! empty($morphMap) && in_array($model, $morphMap)) {
return array_search($model, $morphMap, true);
}
return $model;
}

@inxilpro
Copy link
Contributor

Actually, looking at the code, you really need to replicate the behavior of HasRelationships::getMorphClass because otherwise, as it stands, this will return the wrong value for a model that isn't registered in the morph map.

@GrahamCampbell GrahamCampbell changed the title Add support for getting Model morphed alias [8.x] Add support for getting Model morphed alias Sep 17, 2021
@micc83
Copy link
Contributor Author

micc83 commented Sep 17, 2021

@inxilpro I'll give you my opinion:

HasRelationships::getMorphClass has a very different scope than $query->getModel()->getMorphClass().
While the second one must take in consideration both scenarios (with/without alias), the first one is called only when you're specifically working with aliases so no point for all the additional checks.

Also getMorphedAlias should return false (or null) when statically called on the Relation. The null/false return value will indeed be an indication that the alias is missing for the given model. If no alias is set I would surely not expect for getMorphedAlias to return the model class name.

In the example above, for example, I might check that the alias actually exists or throw an Exception, it would be pretty ugly to have to check for the alias to be equal to the current class name to throw it:

/** @property string $type */
abstract class Parent extends Model {
    protected $table = 'parent_table';
    public static function boot()
    {
        parent::boot();

        static::addGlobalScope(function (Builder $query) {
            $alias = Relation::getMorphedAlias(static::class);
            if (!$alias){ // not $alias === static::class
                throw new Exception('Missing alias for ' . static::class);
            }
            $query->where('type', $alias);
        });
    }
}

Also, for consistency, getMorphedModel returns null if the alias has no matches, not the alias itself (which could be an actual class name for what we know).

@inxilpro
Copy link
Contributor

Also getMorphedAlias should return false (or null) when statically called on the Relation. The null/false return value will indeed be an indication that the alias is missing for the given model. If no alias is set I would surely not expect for getMorphedAlias to return the model class name.

I'm not sure that makes sense. Take your example:

$query->where('type', Relation::getMorphedAlias(static::class)

If there is no morph map for static::class this will turn into:

where type = 0

…which I don't think is the desired behavior. Wouldn't you want it to fall back on the class name in the same way the rest of the framework does, and then rely on something like the new enforceMorphMap flag if that's what you want?

@micc83
Copy link
Contributor Author

micc83 commented Sep 17, 2021

@inxilpro

My first example was indeed naive and the new implementation:

$alias = Relation::getMorphedAlias(static::class);
if (!$alias){ // not $alias === static::class
    throw new Exception('Missing alias for ' . static::class);
}
$query->where('type', $alias);

already gives a better clue of what I meant. About enforceMorphMap, it's definitely out of scope as it's enforced globally.

Also, in this context, where type = 0 is equally bad as where type = 'App\User' as it's simply not what I meant. At least when checking the query anybody would immediatly recognize something is wrong seing type = 0, not sure about the latter.

That's why I think it make sense having getMorphedAlias behave differenty from $query->getModel()->getMorphClass(). They simply act on different levels and contexts. To close it, I think that if getMorphedAlias shall simply replicate what getMorphClass already does it's pointless implementing it at all.

Let's see what others think.

@inxilpro
Copy link
Contributor

Hm. I would definitely expect Relation::getMorphedAlias(User::class) to return the same thing as (new User()->getMorphClass(), and I could see this biting someone if it doesn't. Your initial example shows exactly why: someone may build a query using Relation::getMorphedAlias assuming that it's safe, when it actually may not return the value that they need in their query.

I could see a case for hasMorphedAlias and getMorphedAlias, where getMorphedAlias returns the same as Model:: getMorphClass and hasMorphedAlias returns, essentially Model::getMorphedAlias($class) !== $class.

@micc83
Copy link
Contributor Author

micc83 commented Sep 17, 2021

@inxilpro agree to disagree. Method name explicitly cites "alias" not "class", return types are unequivocal, while the (very simple) method implementation is one click away for anyone with a modern IDE. I actually don't see your point but that's ok. Let's see what others think.

@taylorotwell
Copy link
Member

I think (new Model)->getMorphClass() is sufficient to be honest. I'm not sure we need another method for this. If you would like one you could always define a static helper method on a base model class for your application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants