Skip to content
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.

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

[9.x] Improve missing attributes functionality #44845

Closed
wants to merge 8 commits into from

Conversation

bert-w
Copy link
Contributor

@bert-w bert-w commented Nov 4, 2022

This is an improvement for the recently merged #44283.

The following changes are included:

  1. I got an error in my own project because I was using the HasAttributes trait for a custom class. However, the following piece of code makes a call to the properties $this->exists and $this->wasRecentlyCreated first which did not exist in my implementation, therefore throwing an "Undefined property" ErrorException:

protected function throwMissingAttributeExceptionIfApplicable($key)
{
if ($this->exists &&
! $this->wasRecentlyCreated &&
static::preventsAccessingMissingAttributes()) {
if (isset(static::$missingAttributeViolationCallback)) {
return call_user_func(static::$missingAttributeViolationCallback, $this, $key);
}
throw new MissingAttributeException($this, $key);
}
return null;
}

That if-statement has now been reordered to first call the static::preventsAccessingMissingAttributes(), and since this is false by default the code will work correctly.

  1. I moved all associated code to a new concern so this functionality becomes slightly more scoped instead of being added to the 2k+ lines monsterclass HasAttributes.
  2. I updated a couple calls in the Model class where these functions were used, so it can now work without the trait.

@bert-w bert-w marked this pull request as ready for review November 4, 2022 23:12
@taylorotwell
Copy link
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

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.

2 participants