laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.39k stars 10.98k forks source link

Nullable MorphTo problem since 5.7.20 #27175

Closed dtvmedia closed 4 years ago

dtvmedia commented 5 years ago

Description:

Since the update to 5.7.20 we have problems with our nullable morphTo relations. For example we have an relation for the last editor of an record which uses such relation because we support different kind of users. Means we have these 2 columns:

- updated_from_id
- updated_from_model

Our relation looks like the following:

public function updatedFrom()
{
     return $this->morphTo( null , 'updated_from_model' , 'updated_from_id' );
}

Due to the fact that the database columns are nullable we expect that we can set the updated_from_model column to NULL. But if we do so, we get an SQL Error by calling the relation (e.g. for an article):

ErrorException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'articles.' in 'where clause'
(SQL: select * from `articles` where `articles`.`` = 0 and `articles`.`deleted_at` is null limit 1)

If we set the update_from_model to a valid model and leave the updated_from_id column 0 it works fine again.

Expected Result

Up to version 5.7.19 the result would be just NULL when calling the relation if the update_from_model is NULL.

driesvints commented 5 years ago

@staudenmeir is this related to https://github.com/laravel/framework/pull/27031 ?

dallincoons commented 5 years ago

We've had the same problem which we fixed by explicitly passing the key, but it's a shame that a minor release had breaking changes like this.

staudenmeir commented 5 years ago

You have a model where updated_from_model is null but updated_from_id isn't?

bastian-schur commented 5 years ago

You have a model where updated_from_model is null but updated_from_id isn't?

This is a very good idea, if I set the updated_from_id to null the problem is solved for me

dtvmedia commented 5 years ago

You have a model where updated_from_model is null but updated_from_id isn't?

Yeah I admit that's probably not 100% correctly on our end because the updated_from_id is 0 in this specific case. Nevertheless I think it shouldn't fail if with the given morph combination no model could be retrieved (which should include that updated_from_model is null). Instead it should return just null as before or is there a particular reason for this new behaviour?

y-morozov commented 5 years ago

The problem is $ownerKey is never assigned in case of type field is null.

Here:

return empty($class = $this->{$type})
    ? $this->morphEagerTo($name, $type, $id, $ownerKey)
    : $this->morphInstanceTo($class, $name, $type, $id, $ownerKey);

morphInstanceTo() fills $ownerKey by calling $instance->getKeyName(), and morphEagerTo is not.

noahzgard commented 4 years ago

As i understand problem happens after changes in src/Illuminate/Database/Eloquent/Relations/MorphTo.php, to be precise after removing of this function

    /** 
     * Get the results of the relationship. 
     *  
     * @return mixed    
     */ 
    public function getResults()    
    {   
        return $this->ownerKey ? parent::getResults() : null;   
    }
driesvints commented 4 years ago

@dtvmedia I'm re-looking into this. Can anyone please post full steps to reproduce this problem?

dtvmedia commented 4 years ago

@driesvints Back then the problem was that in case updated_from_id was 0 and updated_from_model was null the morphTo relation would fail (see initial example). Before the Update to version 5.7.20 this combination would return just null. As this issue is over an year old I have no current test case. Back then we switched the updated_from_id to null as suggested which "fixed" it for us.

driesvints commented 4 years ago

I think that's indeed the proper solution.

sinemacula commented 2 years ago

I think that's indeed the proper solution.

Hi @driesvints - I do not believe this issue is fixed, and if I have understood the suggested solution correctly, this does not resolve the issue.

The issue is quite easy to replicate; Just create a nullableUuidMorphs in a migration, and then try to query that relation (use morphTo to define the relation).

For example, this is my relation on my Referral model:

/**
 * Get the referrer.
 *
 * @return \Illuminate\Database\Eloquent\Relations\MorphTo
 */
public function referrer(): MorphTo
{
    return $this->morphTo();
}

Let's assume that my Referral model does not have a referrer i.e. referral_type and referral_id are both null...

If I call $referral->referrer, the expected result would be null - this WORKS If I call $referral->referrer()->first() - this throws the following exception

^ "SQLSTATE[42S22]: Column not found: 1054 Unknown column 'referrals.' in 'where clause' (SQL: select * from referrals where referrals.`` is null limit 1)"

I'm slightly surprised that no one else has run into this over the past few years, surely nullable morphs are not that uncommon? Anyway, is this intended behaviour or is it a bug with Laravel? Or dare I say it, have I done something wrong?

I am actually using Lumen but I don't think that makes a difference as they both use Eloquent... I am currently using version 8.3.4 (Lumen)

sinemacula commented 2 years ago

@driesvints - further to my above comment, I have managed to make some progress. As @y-morozov mentioned, it appears to be to do with the $ownerKey. If update my relation to the following:

/**
 * Get the referrer.
 *
 * @return \Illuminate\Database\Eloquent\Relations\MorphTo
 */
public function referrer(): MorphTo
{
    return $this->morphTo(null, null, null, 'id');
}

It no longer throws the exception... This seems odd but I guess it may be the solution...

mokhosh commented 4 months ago

@driesvints I had this issue and fixed it using sinemacula's approach.

Will there be a cleaner approach to fix this? Should we create a new issue for this?