Skip to content

[13.x] Skip allocation in mergeFillable/Appends/Hidden/Visible when input is empty#60008

Merged
taylorotwell merged 1 commit intolaravel:13.xfrom
olivier-zenchef:fix/eloquent-merge-empty-shortcircuit
May 7, 2026
Merged

[13.x] Skip allocation in mergeFillable/Appends/Hidden/Visible when input is empty#60008
taylorotwell merged 1 commit intolaravel:13.xfrom
olivier-zenchef:fix/eloquent-merge-empty-shortcircuit

Conversation

@olivier-zenchef
Copy link
Copy Markdown
Contributor

@olivier-zenchef olivier-zenchef commented May 6, 2026

Problem

PR #59404 (released v13.3.0, fixing trait initializer collision) replaced if (empty($this->fillable)) { $this->fillable = ... } (and the analogous empty-checks for appends, hidden, visible) with calls to $this->mergeFillable(...) / mergeAppends(...) / mergeHidden(...) / mergeVisible(...).

The collision fix is correct and necessary.

However, those merge* methods are now invoked on every model construction, including when the input array is empty (the common case — most models don't declare #[Appends], so static::resolveClassAttribute(Appends::class, 'columns') ?? [] evaluates to []).

The current implementation:

public function mergeFillable(array $fillable)
{
    $this->fillable = array_values(array_unique(array_merge($this->fillable, $fillable)));
    return $this;
}

For empty $fillable, this still allocates three transient arrays per call (array_merge, array_unique, array_values) — and the same for mergeAppends, mergeHidden, mergeVisible. At high construction rates the cumulative peak-memory cost is significant.

Reproducible benchmark

laravel-eloquent-bench — pinned point-release skeletons, identical synthetic 120-fillable / 13-cast / 50-relation / 4-trait model, N=100,000 constructions on PHP 8.4.14:

Version Peak memory Δ vs v13.1.1
v13.1.1 148 MB
v13.2.0 148 MB 0
v13.3.0 (#59404 lands) 400 MB +252 MB (+170%)
v13.6.0 400 MB +252 MB

Memory recovers to the v13.1.1 baseline (148 MB) when the four merge* calls short-circuit on empty input. At scale on applications that hydrate many models per request, this delta is enough to exceed PHP's default memory_limit.

Fix

Add an early return when the input is empty. Four guards:

public function mergeFillable(array $fillable)
{
    if ($fillable === []) {
        return $this;
    }
    $this->fillable = array_values(array_unique(array_merge($this->fillable, $fillable)));
    return $this;
}

…and the same pattern for mergeAppends, mergeHidden, mergeVisible.

Behavior is preserved: array_values(array_unique(array_merge($x, []))) re-normalizes $x. Skipping that re-normalization when there's nothing to merge is a no-op for any developer-set $fillable / $appends / $hidden / $visible (which are conventionally already clean arrays). The trait-initializer-collision fix that motivated #59404 is preserved — it only matters when there's something to merge, which the guard explicitly skips.

Tests

Added four test_merge_*_with_empty_array_is_noop cases in tests/Database/DatabaseEloquentModelAttributesTest.php asserting:

  • Calling each merge* method with [] returns $this.
  • The underlying property is unchanged (same array reference).

Existing tests for the trait-initializer-collision fix from #59404 continue to pass.

References

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

Thanks for submitting a PR!

Note that draft PRs are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@olivier-zenchef olivier-zenchef marked this pull request as ready for review May 7, 2026 15:07
@taylorotwell taylorotwell merged commit 5bb7394 into laravel:13.x May 7, 2026
54 checks passed
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