Breaking change in relationships with additional where clauses due to PR #25240

[RikSomers]

• Laravel Version: 5.6.34 and above
• PHP Version: 7.1.x
• Database Driver & Version: -

Description:

Our tests started failing after #25240 was merged and tagged. This change was first tagged in 5.6.34

The problem we are having is that we are working with a legacy database that has some tables that have a structure that is similar to composite primary keys. As Laravel does not have support for composite primary keys, we worked around this by providing a where on the relationship that would append the second part of the "composite" primary key, like this:

class TestModel extends Model
{
    protected $primaryKey = 'identifier_1';

    public function relatedModel() {
        return $this->hasOne(RelatedModel::class, 'identifier_1', 'identifier_1')
            ->where(['identifier_2', '=', $this->identifier_2]);
    }
}

Then, somewhere later on we fetch a collection of models and then we need to lazy eager load the relationship on one of the models like this:

$collection = TestModel::where('x', '=', 'value')->get();

// ...

$collection->first()->load('relatedModel');

This has worked up until 5.6.33.

Since the change made in #25240, however, this is not possible anymore because a new instance of the TestModel is created, which loses all attributes that were there before.

The $this->identifier_2 in ->where(['identifier_2', '=', $this->identifier_2]); will therefor be null and will return the wrong related model, or no model at all.

This has been reported (with a PR) before: #25296 but this has been closed by the author, yet the breaking change is still present.

Steps To Reproduce:

See: https://github.com/RikSomers/LaravelRelationBug

Clone the repo then run the unit-test.

As is, they will fail because the composer.json will get the latest tag for laravel\framework:
"laravel/framework": "5.6.*"

Now, edit the composer.json to fetch laravel\framework version 5.6.33:
"laravel/framework": "5.6.33"

Run the tests again, and the tests will pass, proving the breaking change was introduced with Laravel 5.6.34 and above.

[fitztrev]

I closed the referenced issue (#25296) because I realized you really can't do eager loading on a relationship defined like that.

See this package's README where it talks about the problem and then see its linked "Related discussions" for further explanation: https://github.com/topclaudy/compoships

[RikSomers]

Well, as you can see when you clone the repo in my "Steps to reproduce", it works on 5.6.33 and below for our use-case at least.

So either it was a bug with unintended behavior that made it work for our use-case, or something broke with the merging of #25240 .

In any case, I'll take a look at the compoships package.

[RikSomers]

So, is this officially considered an unintended side-effect from a bug that has now been fixed?

[devinfd]

I just spent the past a few hours diagnosing a bug after a composer update. I have found that we are also having the same issue described above. Specifically, our hasOne composite relationship fails to load when a model is restored within SerializesModels.

Reverting Illuminate/Database/Eloquent/Builder line 546 to

return $this->getModel()->{$name}();

returns normal behavior.

[RikSomers]

@devinfd Correct, this is also the line we found in #25252 that needed reverting.

[duxthefux]

Is that going to be working again or is the official statement really „side-effect of the old code“?

[RikSomers]

I'm pretty sure that at this point we can just consider this a bug that has since been fixed, which is a shame.

[devinfd]

Yes. It's a shame that this behavior was "supported" and then suddenly dropped.

[dbpolito]

I don't think this side effect is acceptable as it may harm lots of users with no further notice, i think the laravel team just haven't had the time to look at this.

We probably can fix it in a way to solve the problem for MorphTo use case and still work as it was.

When i get time i will further investigate and maybe open a PR with a solution, but for now i downgraded to 5.6.33.

[driesvints]

Is this still a problem for 5.7.*? We don't support bug fixes for 5.6 anymore but could perhaps take a look at fixing this for 5.7 (if this is fixable).

[RikSomers]

Changing to 5.7.* on the given test repo still produces the same effect, yes.

[fitztrev]

@RikSomers Your repo with steps to reproduce does not do a HasMany relationship, which is where you would see why it's not meant to work like that. The Compoships package does a better job explaining and illustrating the problem that I can.

[RikSomers]

@fitztrev We have since changed our code to something similar to compoships, but written in-house for our needs. But seeing that multiple people (across multiple issues) have had problems with this, I just wanted to provide as much information as I could provide in relation to our use-case in the hopes something could be provided to users that have similar needs. The repo and steps are in itself a simplified example of what did work before and has been broken since, so I will agree in that it is not a repo full of tests for all types of relationships.

If the compoships package provides additional information on why this should or shouldn't work I would advice @driesvints to take a look at the package for more information, but I have not worked with the package myself.

[devinfd]

We have also since had to change our code to something similar to compoships, thereby adding complexity. It would be great if this ability was returned to Laravel 😄

[tbruckmaier]

@driesvints Since the pull request was also backported for v5.5 (#25252), the problem exists in >= v5.5.43 too. (v5.5.42 works fine)

[philipgunther]

Same problem here:

class Folder extends Model
{
    public function path()
    {
        return $this->hasMany(self::class, 'team_id', 'team_id')
            ->where('lft', '<', $this->lft)
            ->where('rgt', '>', $this->rgt)
            ->orderBy('lft', 'asc');
    }
}

class MetadataController extends Controller
{
    public function show(MetadataShowRequest $request, Document $document)
    {
        // Folder path
        optional($document->folder)->load('path');
    }
}

This is a workaround I've found:

class MetadataController extends Controller
{
    public function show(MetadataShowRequest $request, Document $document)
    {
        // Folder path
        optional($document->folder)->path;
    }
}

So this functionality is more in a limbo now. It just doesn't work when called via load.

[giovannipds]

Does this have something related to #16217 or not?

[philipgunther]

Does this have something related to #16217 or not?

No, the issue here is that the model data is not exposed when called via load.

[EspadaV8]

No movement on this issue? Just been trying to update to the latest version of 5.5(.44) to support PHP 7.3 and am now stuck using PHP 7.2 and Laravel 5.5.42. I'll add it to the list of "breaking changes in patch releases."

@RikSomers / @devinfd would you mind sharing examples of what you changed to get things working for you again? Thanks

[RikSomers]

@EspadaV8 Basically we implemented https://github.com/topclaudy/compoships but with a couple of changes to better suit our needs. But you can probably just use compoships as-is and be fine.

[driesvints]

Since Laravel doesn't (and hasn't ever afaik) supported composite keys and this issue is quite old I'm closing this. If anyone ever wants to try to tackle implementing support for composite keys feel free to open up an issue on the ideas repo to discuss how we'd best implement it while at the same time not introducing any breaking changes into Eloquent. Thanks for all the feedback here, hopefully someone can make this possible someday.

[Wulfheart]

This bit me today. :(