spatie / laravel-query-builder

Easily build Eloquent queries from API requests
https://spatie.be/docs/laravel-query-builder
MIT License
4.02k stars 395 forks source link

Remove relation name conversion and respect included fields for relations #794

Closed dominikb closed 1 year ago

dominikb commented 2 years ago

This PR addresses two issues:

I've tried to split them originally but the changes go hand-in-hand which is why I decided against it in the end.

Including a relation now requires specifying the exact relation name. This should clear up some confusion that arose with the automatic conversion in the past.

The key and foreign key columns are always queried and included automatically as they are required to associate the loaded models. I don't know if this automatic resolution is something we want to support as I have a feeling that it won't always be possible.

class TestModel extends Model
{
    public function relatedModels(): HasMany
    {
        return $this->hasMany(RelatedModel::class);
    }
}

// Previously
->allowedIncludes('relatedModels')
->allowedFields('related_models.name')

// Now
->allowedIncludes('relatedModels')
->allowedFields('relatedModels.name') //  Also automatically includes relatedModels.id

I've added a test case to verify that included fields for relations are respected, but I'm uncertain how the implementation holds up with more complex model setups. As I don't currently use this package in any of my projects, I can't test it extensively.

Please have a look at it and maybe test the changes. Any feedback or criticism is welcome.

beonami commented 2 years ago

This pull request is pending?

leandrodiogenes commented 2 years ago

Thank you so much @dominikb, this is exacly what im looking for. Waiting the pull request aproval.

VapoFred78 commented 1 year ago

Hello, Will this pull request be merged one day?

spatie-bot commented 1 year ago

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

andrewfloripa commented 11 months ago

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

Regarding the correction reported by Dominikb about the plural.

Why was there no pull request? Because I'm facing the same issue. I have a relationship with a singular name since it's a HasOne, and in the filter, I have to add an "S", forcing an incorrect plural outside of the Laravel convention.

leandrodiogenes commented 11 months ago

+1

carvemerson commented 8 months ago

Any chance to have this changes merged? @dominikb @AlexVanderbist

VinceBoul commented 6 months ago

Same issue here, when would these changes can be merged ? @dominikb @AlexVanderbist