Skip to content

Add support for custom morph class resolvers #13

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

Merged
merged 5 commits into from
Apr 28, 2023

Conversation

misenhower
Copy link
Contributor

@misenhower misenhower commented Apr 19, 2023

Hey there! This PR adds an optional resolveUsing setting to the morph map generator.

This is a similar idea to what was proposed in #3, but enables users to customize how the "default" alias is resolved by providing a callback.

For instance, you could base the morph names off the table name for each model like this:

MorphMapGenerator::resolveUsing(fn ($model) => Str::singular($model->getTable()));

This would generate names like post or comment depending on the model's table name in the DB (which should be pretty stable even if you rearrange your model classes). This is just an example though, and you could add whatever logic you might want there.

Let me know what you think. Thanks!

Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

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

Thanks! Could you also mention this feature in the readme and a clear example?

@@ -39,7 +46,9 @@ private function resolveMorphFromModelClass(ReflectionClass $reflection): ?array
$model = $reflection->newInstanceWithoutConstructor();

try {
$morph = $model->getMorphClass();
$morph = static::$resolveCallback
? call_user_func(static::$resolveCallback, $model)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to support having the resolveCallback returning null. When null is returned, we could fallback to getMorphClass

@rubenvanassche
Copy link
Member

rubenvanassche commented Apr 20, 2023

Thanks for the PR but I don't think is a feature we want to add? The whole point of a morph map is setting things in stone, with this PR you allow things that can be changed to be used as morph keys and thus your morph map won't be stable again.

In you example, the table name can be changed at a point in time and thus there is no validity in your morph map. This example is indeed quite stable, but it happens that your table names change. That will be quite a nasty bug to find would this happen.

What do you think @freekmurze?

@freekmurze
Copy link
Member

I'm always up for a little flexibility, but I agree there's value in following the strict spirit of this package, and pass up on this.

@misenhower
Copy link
Contributor Author

My thinking is: if you're changing a DB table name, you're already making significant changes to your DB, so you'd probably expect to have to make changes to things like this as well.

There's a lot of functionality in Laravel that derives table/column names from class names, method names, etc. so IMO this fits in with the overall Laravel "philosophy" but I understand your concerns as well :) It's an opt-in feature though, so anyone with concerns about stability wouldn't have to use it.

In any case, I've made the requested changes in case you do choose to merge this. Thanks!!

@misenhower misenhower requested a review from freekmurze April 20, 2023 18:06
@rubenvanassche
Copy link
Member

Let's merge it in with a strong warning 😄

@rubenvanassche
Copy link
Member

Thanks

@rubenvanassche rubenvanassche merged commit ea13221 into spatie:main Apr 28, 2023
@misenhower
Copy link
Contributor Author

Hey, thanks for merging this! Much appreciated 😃

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.

3 participants