[5.1] [5.2] Polymorphic relation naming is inconsistent

[joostbaptist]

Hi,

Like #9427, I'm running into some strange naming behaviour with polymorphic relations. I would like to ask you to look at this again.

My polymorphic relation is defined as follows in class Module:

public function moduleInstance()
{
    return $this->morphTo();
}

Now, in order to load this relation, I have to call:

$module->load('moduleInstance');

However, to retrieve this relation, I have to call:

$module->module_instance

This clearly shows that naming is inconsistent. All relations are named using camel case and it seems that only polymorphic relations are named using snake case.

Here is another example that clearly shows this inconsistent behaviour:

// moduleType is another, BelongsTo relation that is named using camel case
$module->load(['moduleInstance', 'moduleType']);
dd($module);

Below is a screenshot that shows the result of executing the above code:

Also note that another empty relation ('moduleInstance') is added using an actually consistent name.

Thanks in advance!

[joostbaptist]

To clarify, if I don't eager load the relation first, then naming is consistent (using camel case).

[rkgrep]

Did you try to change static::$snakeAttributes value of your model?

[zakhenry]

👍 this seems to be an issue only with morphTo relation

[zakhenry]

I suspect the issue is at

framework/src/Illuminate/Database/Eloquent/Relations/MorphTo.php

Line 107 in 10e1d08

return $models;

:

    /**
     * Match the eagerly loaded results to their parents.
     *
     * @param  array   $models
     * @param  \Illuminate\Database\Eloquent\Collection  $results
     * @param  string  $relation
     * @return array
     */
    public function match(array $models, Collection $results, $relation)
    {
        return $models;
    }

• compare the method with the parent method in belongsTo (

  framework/src/Illuminate/Database/Eloquent/Relations/BelongsTo.php

  Line 196 in 10e1d08

  public function match(array $models, Collection $results, $relation)

  ) -

/**
     * Match the eagerly loaded results to their parents.
     *
     * @param  array   $models
     * @param  \Illuminate\Database\Eloquent\Collection  $results
     * @param  string  $relation
     * @return array
     */
    public function match(array $models, Collection $results, $relation)
    {
        $foreign = $this->foreignKey;
        $other = $this->otherKey;
        // First we will get to build a dictionary of the child models by their primary
        // key of the relationship, then we can easily match the children back onto
        // the parents using that dictionary and the primary key of the children.
        $dictionary = [];
        foreach ($results as $result) {
            $dictionary[$result->getAttribute($other)] = $result;
        }
        // Once we have the dictionary constructed, we can loop through all the parents
        // and match back onto their children using these keys of the dictionary and
        // the primary key of the children to map them onto the correct instances.
        foreach ($models as $model) {
            if (isset($dictionary[$model->$foreign])) {
                $model->setRelation($relation, $dictionary[$model->$foreign]);
            }
        }
        return $models;
    }

the method seems suspiciously empty for what should be more complex than BelongsTo?

[zakhenry]

@joostbaptist I've come up with a solution, see PR #10864 , however I'm having a hard time understanding the logic in the unit test, I'll have another crack at it tomorrow, but I'm very open to suggestions on how to fix DatabaseEloquentMorphToTest::testModelsAreProperlyPulledAndMatched

My change moves the registration of related models from the load eager into the match function, which is in line with the rest of the relationships, however the unit test is expecting the original order. Maybe a unit test rewrite is in order.

[GrahamCampbell]

Closing since there is a PR now.

[joostbaptist]

Hi all, thank you for looking into this. I found that changing the morphTo method in the Model class (https://github.com/laravel/framework/blob/5.1/src/Illuminate/Database/Eloquent/Model.php#L847-L881) as shown below fixes the problem for me, although I'm not sure that it won't cause other problems. Thought I'd share this anyway, in case it might be helpful.

public function morphTo($name = null, $type = null, $id = null)
{
    // Get the name of the relation from the function name.
    list($current, $caller) = debug_backtrace(false, 2);
    $relation = Str::camel($caller['function']);

    // If no name is provided, we will use the name of the relation
    // since that is most likely the name of the polymorphic interface. We can
    // use that to get both the class and foreign key that will be utilized.
    if (is_null($name)) {
        $name = Str::snake($relation);
    }

    list($type, $id) = $this->getMorphs($name, $type, $id);

    // If the type value is null it is probably safe to assume we're eager loading
    // the relationship. When that is the case we will pass in a dummy query as
    // there are multiple types in the morph and we can't use single queries.
    if (is_null($class = $this->$type)) {
        return new MorphTo(
            $this->newQuery(), $this, $id, null, $type, $relation
        );
    }

    // If we are not eager loading the relationship we will essentially treat this
    // as a belongs-to style relationship since morph-to extends that class and
    // we will pass in the appropriate values so that it behaves as expected.
    else {
        $class = $this->getActualClassNameForMorph($class);

        $instance = new $class;

        return new MorphTo(
            $instance->newQuery(), $this, $id, $instance->getKeyName(), $type, $relation
        );
    }
}

Note that I use the backtrace to extract the name of the relation ($relation, which in my use case would be 'moduleInstance'), and when instantiating the MorphTo class, this relation name is passed instead of the name of the polymorphic property ($name, which would be 'module_instance').

[zakhenry]

Hey @joostbaptist, thanks for the update, to fill you in on the progress since this issue was last closed - I found the issue to be the api is inconsistent with when it registers the eager loading dictionary for morphTo, and so I corrected the issue with the PR #10864

This PR was merged and released, but then in #11138 it was found that my fix has somehow caused memory issues, which is surprising as the all the fix really does is align the morphTo relation with the rest of the relationships. Anyhow as we were unable to replicate this memory issue with tests, the PR was reverted, and this issue was re-opened.

Your solution might actually end up being the best option, as it doesn't affect the way the relationship works, and so should not cause the memory issue.

Note that you can just do $class::getPrimaryKey() instead of creating a new instance (though that's what it does under the hood anyway).

[xian13]

Hi All,

Is it possible to remove the Str::snake here

and just use the function caller name?

The problem in consistency is the snake case the function caller.