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

Callback of Model::handleMissingAttributeViolationUsing called when using isset to check if attribute exists #44834

Closed
gdebrauwer opened this issue Nov 3, 2022 · 3 comments

Comments

@gdebrauwer
Copy link
Contributor

gdebrauwer commented Nov 3, 2022

  • Laravel Version: 9.38.0
  • PHP Version: 8.1.3

Description:

When a custom callback is defined to handle missing attribute violations (introduced in #44664), then that callback is also called when using the isset function to check if an attribute exists. That is not correct, because it is not a violation when using isset

Steps To Reproduce:

Model::shouldBeStrict(true);

Model::handleMissingAttributeViolationUsing(function ($model, $key) {
    dump('this should not be called!');
});

$user = User::factory()->create();

if (isset($user->fresh()->foobar)) {
  // ...
}
@driesvints
Copy link
Member

We have no plans anymore to change this behavior, please see the convo at #44717

@gdebrauwer
Copy link
Contributor Author

The discussion in that PR seems to be around something else. This issue is caused by the fact that the offsetExists method tries to catch an exception that will never be thrown. The catch makes sense when no missingAttributeViolationCallback is defined, but when there is callback defined the catch does nothing.

public function offsetExists($offset): bool
{
try {
return ! is_null($this->getAttribute($offset));
} catch (MissingAttributeException) {
return false;
}
}

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;
}

@sebastiaanluca
Copy link
Contributor

sebastiaanluca commented Dec 10, 2022

Got bit by the same bug, see #44642 (comment). Was a really nice feature, but unusable unless you always directly access the attribute and don't need to check for its existence.

In this case, if you for instance report these on production, they're basically false-positives and don't need to be reported. Yet there's no option to disable the reporting or callback when accessed via isset() or ??.

I tried to work around it using various approaches, but haven't found a viable solution yet. So it's either strict mode and no reporting (not usable on production), or no strict mode (buggy app) 🥲

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

No branches or pull requests

3 participants