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

GateCollector::addCheck errors when target is a model with no key #1688

Closed
sparksp opened this issue Oct 12, 2024 · 8 comments · Fixed by #1691
Closed

GateCollector::addCheck errors when target is a model with no key #1688

sparksp opened this issue Oct 12, 2024 · 8 comments · Fixed by #1691

Comments

@sparksp
Copy link

sparksp commented Oct 12, 2024

I believe that #1653 introduced an undesirable bug when using a Model with no key as the target for a gate check.

It presents itself as The attribute [id] either does not exist or was not retrieved for model [App\Models\ModelName]. and took me quite a while to realise it was a problem with debug bar, and not with my code, as the vendor trace is hidden by default.

I have a model Attendance which extends from Illuminate\Database\Eloquent\Relations\Pivot. Instances are unique by the foreignKey and relatedKey. AsPivot handles this internally and makes an isset check for any primary key before setting keys for query manually.

In the case of a Pivot model, you could use the foreign and related keys in the description. But I can imagine a case where a model has been fetched without its id which would still break this feature, so it may be wise to include an isset check of your own and fall back to no description?

@barryvdh
Copy link
Owner

Hmm so what is getKeyName() then? It seems like default Laravel, so weird it would crash. Normally Models don't crash on not-existing properties, right?

@barryvdh
Copy link
Owner

Can you try to make a PR ?

@sparksp
Copy link
Author

sparksp commented Oct 18, 2024

Hmm so what is getKeyName() then? It seems like default Laravel, so weird it would crash. Normally Models don't crash on not-existing properties, right?

In this case getKeyName() is still "id" as that is the default set by Model. But the model has no id, and there is no other sensible attribute I can use here.

I use Model::shouldBeStrict() in all my projects, which will throw an exception when you try to access a missing attribute - this helps me to spot errors in development quickly from typos or overly restrictive queries.

Can you try to make a PR ?

Yes, for sure. I might find some time next week, otherwise I'll have time in early November.

@barryvdh
Copy link
Owner

Can you test #1691 ?

@niladam
Copy link

niladam commented Oct 18, 2024

This for reason creates issues with https://github.com/bezhanSalleh/filament-shield who's using spatie's permission and outputs an error like Call to undefined method App\Models\Role::hasAttribute();

In order for this to be confirmed, toggling the gate collector shows this.

@barryvdh
Copy link
Owner

okay so

 if ($model->getKeyName() && isset($model[$model->getKeyName()])) {`

works?

@niladam
Copy link

niladam commented Oct 18, 2024

Yup. This does seem to fix it :)

@sparksp
Copy link
Author

sparksp commented Oct 20, 2024

Happy to confirm this is working for my case also.

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 a pull request may close this issue.

3 participants