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

Prevent creating non-existent models from container #15160

Open
wants to merge 7 commits into
base: 4.x
Choose a base branch
from

Conversation

ralphjsmit
Copy link
Contributor

@ralphjsmit ralphjsmit commented Dec 21, 2024

Currently, the closure evaluation system works great by resolving any dependencies not provided by name or type from the container. This is very useful. However, there is one situation where it isn't useful and that is when it is evaluating empty/non-existent Eloquent models from the container.

For example, in Filament V3 we had the nice feature of being able to type an Eloquent model as both User $record and User $user, which would both work nicely. However, sometimes there are situations where you think you have a $record, but actually for some reason you don't (or you have a different type of record when working with eg relationships or repeaters). If you would then still type it as User $user, you would get an empty/non-existent Eloquent model from the container. This has caught me in very complex forms out quite a few times and therefore I generally prefer User $record now just for this, whilst for the cleanliness I would prefer User $user.

This PR fixes the issue by preventing the container from instantiating an empty Eloquent model if it was not able to resolve dependency using type or name. This prevents users from typing things like User $user, thinking they have the record but they actually don't. It should not really impact users, as no one would instantiate their empty models using the container and would only help to prevent bugs. I also added a test to prove the behavior.

Thanks!

@ralphjsmit
Copy link
Contributor Author

There are some changes in un-touched files because of Pint.

if (filled($typedParameterClassName)) {
if (
filled($typedParameterClassName)
&& ! is_subclass_of($typedParameterClassName, Model::class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the model has been bound to the container manually, we should still resolve it. Someone might want to bind the current tenant to the container for easy access, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, changed!

@danharrin danharrin added the bug Something isn't working label Dec 24, 2024
@danharrin danharrin added this to the v4 milestone Dec 24, 2024
@ralphjsmit ralphjsmit force-pushed the rjs/prevent-evaluating-non-existent-models-from-container branch from 19787a3 to 006d84e Compare December 26, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting next major version bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants