8000 [12.x] Retain associative keys on eager loaded relations by Jade-GG · Pull Request #58506 · laravel/framework · GitHub
[go: up one dir, main page]
Skip to content

[12.x] Retain associative keys on eager loaded relations #58506

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.

Sign up for GitHub

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

Jump to bottom
Open
Jade-GG wants to merge 3 commits into
base: 12.x
Choose a base branch
from
Open

[12.x] Retain associative keys on eager loaded relations #58506

Jade-GG wants to merge 3 commits into from
+63 −13

Conversation

@Jade-GG
Copy link
@Jade-GG Jade-GG commented Jan 26, 2026 •
edited
Loading

Fixes #57754.

TL:DR; this specifically arose from a very niche use case in one of our projects, where you may try to use the (currently undocumented) afterQuery function as part of an eager loaded relation, in which that afterQuery causes your collection to get become an associative collection. For example:

public function posts(): HasMany
{
    return $this
        ->hasMany(Post::class)
        ->afterQuery(fn($posts) => $posts->keyBy('id'));
}

If you use lazy loading, e.g., User::first()->posts, this already worked as I would have expected. However, when using eager loading, e.g., User::with('posts')->first()->posts the resulting array would become non-associative due to how the dictionary is being built. This PR changes that behaviour specifically for the case that the resulting array out of a relation is associative.

This (as far as I can tell) will not have any bad side effects as I've made sure this will only occur on non-associative arrays and only specifically in the dictionaries of relations. Non-associative arrays there won't normally occur unless you specifically want them to be there.

Of course I don't have the most in-depth knowledge about the inner workings of the core framework, so I could be wrong, however I wasn't able to find any counterexamples.

@dxnter
Copy link
Contributor
dxnter commented Jan 26, 2026

I understand why you modified Collection::mapToDictionary() to let HasOneOrMany::buildDictionary() just work, but I would be concerned about changing Collection::mapToDictionary() because:

  1. The docblock contract is violated. It explicitly states array<int, TMapToDictionaryValue> for inner arrays
  2. It's a general purpose collection method. Code outside Eloquent may rely on sequential keys
    • mapToGroups also calls mapToDictionary internally, so the changes cascade to that method too

Instead of modifying mapToDictionary, I think HasOneOrMany::buildDictionary() could be rewritten to match the pattern being used for BelongsToMany, HasOneOrManyThrough, and MorphTo. This would keep the fix contained to Eloquent without risking side effects in unrelated Collection usage.

One other minor note, since keys can be either int, or the original associate key type, array<array<array-key, TRelatedModel>> would be more accurate across all the buildDictionary methods.

@Jade-GG
Copy link
Author
Jade-GG commented Jan 27, 2026

Just to make sure the functionality stayed the same, I rewrote the function by copy-pasting the code from mapToDictionary and simplifying out anything we didn't need here. I've also changed the docblocks as suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Jade-GG @dxnter
0