PHPLARA-32 Fix attribute masked by a set-only Attribute mutator - #3557
Open
GromNaN wants to merge 1 commit into
Open
PHPLARA-32 Fix attribute masked by a set-only Attribute mutator#3557GromNaN wants to merge 1 commit into
GromNaN wants to merge 1 commit into
Conversation
An attribute whose name collides with a method typed `: Attribute` that declares only a `set:` callback was routed to getRelationValue() and always resolved to null. laravel/passport Client::secret() is such a case, which made Client::confidential() unable to read the secret. DocumentModel::getAttribute() guarded the embedded relation branch with hasAttributeGetMutator(), which is false when no `get:` callback is set. Use hasAttributeMutator() instead, matching what Laravel core uses in both hasAttribute() and isRelation().
There was a problem hiding this comment.
Pull request overview
This PR fixes an Eloquent attribute-read edge case in DocumentModel where a stored attribute could be incorrectly treated as an embedded relation when there is a colliding : Attribute mutator method that defines only a set: callback (e.g., Laravel Passport Client::secret()), causing $model->attr to return null.
Changes:
- Adjust
DocumentModel::getAttribute()to treat any: Attributemutator (including set-only) as an attribute, preventing the embedded-relation short-circuit. - Add a regression test ensuring a hidden attribute with a set-only
Attributemutator remains readable via property access while still excluded fromtoArray(). - Extend the
HiddenAnimaltest model to include asecretattribute and a set-onlyAttributemutator to reproduce the collision scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Eloquent/DocumentModel.php |
Fixes attribute-vs-relation precedence by switching the guard to hasAttributeMutator() to align with Laravel’s relation detection. |
tests/Models/HiddenAnimal.php |
Adds a hidden secret attribute and a set-only Attribute mutator to reproduce the Passport-like collision. |
tests/PropertyTest.php |
Adds a regression test asserting $model->secret is readable while remaining hidden in serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes PHPLARA-32, reported from laravel/passport#1850 (comment).
An attribute whose name collides with a method typed
: Attributethat declares only aset:callback is unreadable:$model->attralways returnsnull.laravel/passportClientis exactly this shape:Client::confidential()could not read the secret, socreateToken()failed withPersonal access client not found for '$provider' user provider.. Passport worked around it in laravel/passport#1850 by readinggetAttributes()['secret']directly, but any userland code doing$client->secretis still affected.Note this has nothing to do with
$hidden, which only filters serialization. The collision with the mutator method name is what breaks it.Cause
DocumentModel::getAttribute()short-circuits togetRelationValue()when a method matches the attribute name, so that embedded relations take precedence over the raw stored array. It guarded that branch withhasAttributeGetMutator(), which returnsfalsewhen theAttributehas noget:callback. The attribute was therefore treated as a relation.That path was a dead end: Laravel's
getRelationValue()callsisRelation(), which itself returnsfalseas soon ashasAttributeMutator()is true, so it fell through to a barereturn;, hence thenull.The fix uses
hasAttributeMutator(), which is what Laravel core uses in bothhasAttribute()andisRelation(). This removes the disagreement betweengetAttribute()andisRelation().Backward compatibility
hasAttributeMutator()is a strict superset ofhasAttributeGetMutator(), so the only keys taking a different path are methods typed: Attributewhosegetis not callable. I ran a matrix over every method/attribute collision shape, before and after:Attributeget + set'GET:stored''GET:stored'Attributeget only'GET:stored''GET:stored'Attributeset onlynull'stored'Attributewith neithernull'stored'LogicExceptionLogicExceptionLogicExceptionLogicExceptiongetValAttribute()accessor'LEGACY:stored''LEGACY:stored'The two rows that change previously returned
nullunconditionally, so there was no reachable behavior to depend on. A method declared: Attributecan never return aRelationinstance either, so it can never legitimately be an embedded relation.Other points checked:
isset($model->attr)goes fromfalsetotrue, which is the same correction and matches Eloquent on SQL.$hiddenfiltering happens inattributesToArray(), and the new test asserts the attribute stays out oftoArray().hasAttributeMutator()is public, not@internal, not deprecated, and present in bothilluminate/database12.x and 13.x, covering the^12.51|^13.0constraint.->get. The new one only uses reflection, so slightly less work and no userland code executed during an attribute read.Pre-existing limitation left untouched: a plain method whose name collides with a stored attribute still shadows it and throws
LogicException. Making the stored attribute win there would break embedded relations, which rely on that precedence.