laravel-query-builder icon indicating copy to clipboard operation
laravel-query-builder copied to clipboard

Remove relation name conversion and respect included fields for relations

Open dominikb opened this issue 1 year ago • 2 comments

This PR addresses two issues:

  • automatic snake_case conversion of included relations: #792, #440, #640
  • respecting allowed fields for relationship fields: #681

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.

dominikb avatar Jul 27 '22 21:07 dominikb

This pull request is pending?

beonami avatar Aug 01 '22 15:08 beonami

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

leandrodiogenes avatar Aug 31 '22 04:08 leandrodiogenes

Hello, Will this pull request be merged one day?

VapoFred78 avatar Feb 22 '23 09:02 VapoFred78

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.

spatie-bot avatar Jul 11 '23 10:07 spatie-bot

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.

andrewfloripa avatar Sep 19 '23 18:09 andrewfloripa

+1

leandrodiogenes avatar Oct 06 '23 00:10 leandrodiogenes

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

carvemerson avatar Jan 06 '24 22:01 carvemerson

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

VinceBoul avatar Feb 29 '24 14:02 VinceBoul