Skip to content

fix(laravel): Allow LinksHandler to handle polymorphic relationships #7231

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 2 commits into from
Jun 19, 2025

Conversation

jonerickson
Copy link
Contributor

Q A
Branch? 4.1
Tickets #7227
License MIT
Doc PR api-platform/docs#...

This PR adds support for polymorphic relationships from within the LinksHandler.

Previously:

This would result in bad query formation where the model type was not taken into account and you could have multiple model types returned with the same IDs:.

select * from `attachments` where `attachments`.`model_id` = ?

new GetCollection(
    uriTemplate: '/projects/{id}/attachments',
    uriVariables: [
        'id' => new Link(
            fromProperty: 'attachments',
            fromClass: Project::class,
        ),
    ],
),

Now:

select * from `attachments` where `attachments`.`model_type` = ? and `attachments`.`model_id` = ? and `attachments`.`model_id` is not null

We now properly scope the query to the model class as well.

@jonerickson jonerickson force-pushed the morph-many-relations branch from 71f4770 to dc53b75 Compare June 18, 2025 17:44
@jonerickson
Copy link
Contributor Author

jonerickson commented Jun 18, 2025

@soyuka in the first commit, I addressed the underlying issue. In the second commit, I am posposing a better way to handle links - using the built in standard eloquent relationship methods. By simply hydrating the base class with its identifier $relatedInstance->setAttribute($relatedInstance->getKeyName(), $identifier); we can then just call the relationship methods to get the querys - no longer have to build them from scratch. With BelongsTo we obviously have a different situation because the FK lives on the child table so we have to use native joins. I feel this is a much cleaner way moving forward.

@jonerickson jonerickson force-pushed the morph-many-relations branch from 0c30958 to 997fec0 Compare June 18, 2025 18:51
$relationQuery = $relation->{$from}();
if (!method_exists($relationQuery, 'getQualifiedForeignKeyName') && method_exists($relationQuery, 'getQualifiedForeignPivotKeyName')) {
/** @var Model $relatedInstance */
$relatedInstance = $this->application->make($link->getFromClass());
Copy link
Member

Choose a reason for hiding this comment

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

This line makes me think about #7166, its another subject but do you have any idea on how we could do otherwise to instantiate this class?

Copy link
Contributor Author

@jonerickson jonerickson Jun 19, 2025

Choose a reason for hiding this comment

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

I think what needs to happen in the item/collection provider or property accessor is something like loadMissing on the relations that are discovered through the eloquent property metadata service. This error happens when calling a relation that hasn't already been hydrated in the class. By calling loadMissing we can hopefully prevent that. I'll play around with it and see what I can come up with.

Copy link
Contributor Author

@jonerickson jonerickson Jun 20, 2025

Choose a reason for hiding this comment

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

Actually, we don't need to do anything anymore. This switched fix #7166 as well. See #7166 (comment)

@soyuka
Copy link
Member

soyuka commented Jun 19, 2025

This is awesome thank you very much @jonerickson !

@soyuka soyuka merged commit cdda414 into api-platform:4.1 Jun 19, 2025
109 of 111 checks passed
@jonerickson jonerickson deleted the morph-many-relations branch June 20, 2025 15:36
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.

2 participants