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

Relation::tableNameAsMorphType breaks morphTo relations #35519

Closed
dmason30 opened this issue Dec 7, 2020 · 12 comments · Fixed by #35533
Closed

Relation::tableNameAsMorphType breaks morphTo relations #35519

dmason30 opened this issue Dec 7, 2020 · 12 comments · Fixed by #35533
Labels

Comments

@dmason30
Copy link
Contributor

dmason30 commented Dec 7, 2020

  • Laravel Version: 8.17.2
  • PHP Version: 7.4
  • Database Driver & Version: MySQL 8

Description:

The #35257 PR doesn't seem to work with morphTo relations. You end up with an Class 'posts' not found when using in conjunction with Relation::tableNameAsMorphType();

Error : Class 'posts' not found
/src/Illuminate/Database/Eloquent/Relations/MorphTo.php:180
/src/Illuminate/Database/Eloquent/Relations/MorphTo.php:129
/src/Illuminate/Database/Eloquent/Relations/MorphTo.php:115
/src/Illuminate/Database/Eloquent/Builder.php:597
/src/Illuminate/Database/Eloquent/Builder.php:566
/src/Illuminate/Database/Eloquent/Builder.php:534

Steps To Reproduce:

Taking the example from the docs where imageable could be a Post or a User.
https://laravel.com/docs/8.x/eloquent-relationships#one-to-one-polymorphic-relations:

class Image extends Model
{
    public function imageable()
    {
        return $this->morphTo();
    }
}

Add the below to AppServiceProvider in the boot method:

class AppServiceProvider extends ServiceProvider
{
    public function boot()
    {     
        Relation::tableNameAsMorphType();
    }
}

The below code will throw the exception described above:

$post = Post::factory()->create();

$image = Image::factory()->create([
    'imageable_type' => $post->getTable(), // 'posts'
    'imageable_id' => $post->id,
]);

// This will throw the exception
$postTitle = $image->imageable->title
@driesvints
Copy link
Member

Can you create a quick repo that reproduces the problem? Please commit custom code separately from the base skeleton.

@taylorotwell
Copy link
Member

@imanghafoori1

@driesvints
Copy link
Member

I've come to the conclusion that it's impossible to solve this one. Since with the inverse here you only have the context of the alias there's no way to derive the class name from it as you simply don't know the namespace of the model.

At the moment I'm more leaning to reverting this completely as we can't ever ship this properly. Mapping custom morph types will always need to be done manually through the morph map.

@imanghafoori1
Copy link
Contributor

Checking the problem

@imanghafoori1
Copy link
Contributor

imanghafoori1 commented Dec 8, 2020

It seems that the morphTo relation exceptionally does not accept a $related model, in the case Post::class.
And assumes that there is a mapping for it or there is the model path in DB.
So there are a few approaches to this for laravel 9.x :

  • To change the $this->morphTo(... to accept a "$related" parameter, like other morph relations. (also makes the underlying code simpler then)

  • Hold on to this behavior as the default to this and document Relation::morphMap(...); rerquired only for morphOne relations. (Not for all relations)

  • Try to derive \App\Post, \App\Models\Post from posts before bail out with an error.

  • or Revert it 💀 🙌

Thanks, @dmason30 for clarification.

@driesvints
Copy link
Member

  1. Kind of defeats the purpose of morphMap to begin with, 2. would still contain the broken code, 3. isn't feasible (as explained above)

I think it's best to revert this.

@imanghafoori1
Copy link
Contributor

@driesvints Yeah, the first option not possible with one model. it should accept a list of "Imagable models" which is not very good either.

@dmason30
Copy link
Contributor Author

dmason30 commented Dec 8, 2020

One consideration for option 3 is that you can still use Relation::morphMap in conjunction with Relation::tableNameAsMorphType

Say the code was changed to attempt to morph table names into models via str->singular->camelCase and checking class_exists to throw a more meaningful error.

If you had a table 'posts' but for the benefit of this example for some reason your model was called BlogPost then you could setup in the AppServiceProvider.

class AppServiceProvider extends ServiceProvider
{
    public function boot()
    {     
        Relation::tableNameAsMorphType();

        Relation::morphMap(['posts' => BlogPost::class]);
    }
}

The benefit here keeps it using the table name as the default so you don't end up adding all your models to morphMap and if it can't find the model when doing morphTo the error should suggest to add the table => model to the map.

If your models were outside of the default App/ or App/Models/ locations then you probably shouldn't use this method anyway.

If you decide to revert fair enough. I have lived with having a 100 models listed in my morphMap thus far. 🤣

Perhaps the only true solution is what the below package attempts which scans models and caches the morphMap setting the table name as the alias.

https://github.com/sebastiaanluca/laravel-auto-morph-map

@taylorotwell
Copy link
Member

Let's just revert it.

@taylorotwell
Copy link
Member

@driesvints can you send in the revert PR?

@driesvints
Copy link
Member

@taylorotwell will do

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

Successfully merging a pull request may close this issue.

4 participants