Prevented dependency injection of Models (non existent?)
What was the reason for this change: https://github.com/filamentphp/filament/blame/4.x/packages/support/src/Concerns/EvaluatesClosures.php#L85
@ralphjsmit
One can restore this by binding all the models, but this goes against Laravel DI since Models can be injected without issues. A clear commit agains this. I'd like to know the reasons behind this. Thank you.
GitHub
Blaming filament/packages/support/src/Concerns/EvaluatesClosures.ph...
A powerful open source UI framework for Laravel • Build and ship admin panels & apps fast with Livewire - Blaming filament/packages/support/src/Concerns/EvaluatesClosures.php at 4.x · filam...
Solution:Jump to solution
In Filament V3 it introduced that you can now alias the injected record to another parameter name instead of
$record. For example, you could just do Post $post instead of Post $record. Whilst this was convenient, there are quite often situations where you think there should be a record, but there actually is not (e.g. in complex forms with relationships etc). In that case, returning Post $record would fail on that the argument definition does not allow null, prompting a change to ?Post $record. However, without the above change you linked, the Post $post would still return a Post model, but then an empty one that does not exist at all created by Laravel dependency injection. This causes then weird bugs in your code later one if you expect $post to be the current $record and it turns out to be just an empty unsaved model. Therefore, if the requested parameter is an Eloquent parameter that is not of the current record/model, this line has been added to prevent such unwanted empty model resolutions.
See also here: https://github.com/filamentphp/filament/pull/15160...GitHub
Prevent creating non-existent models from container by ralphjsmit ...
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...
19 Replies
Solution
In Filament V3 it introduced that you can now alias the injected record to another parameter name instead of
$record. For example, you could just do Post $post instead of Post $record. Whilst this was convenient, there are quite often situations where you think there should be a record, but there actually is not (e.g. in complex forms with relationships etc). In that case, returning Post $record would fail on that the argument definition does not allow null, prompting a change to ?Post $record. However, without the above change you linked, the Post $post would still return a Post model, but then an empty one that does not exist at all created by Laravel dependency injection. This causes then weird bugs in your code later one if you expect $post to be the current $record and it turns out to be just an empty unsaved model. Therefore, if the requested parameter is an Eloquent parameter that is not of the current record/model, this line has been added to prevent such unwanted empty model resolutions.
See also here: https://github.com/filamentphp/filament/pull/15160GitHub
Prevent creating non-existent models from container by ralphjsmit ...
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...
@ralphjsmit yeah, thanks! I've stumbled across this in my development in V3, but I've just implemented a proper check. So this is just a "convenience" update, because at the end of the day it's the programmers responsibility to catch "nullable"models.
However, since I've already covered such cases, would you agree that binding my models in the register method to get passed the condition to the "old injection" would just revert the situation. Looking at the code though, you've deliberately added the check to see if it's bound in the container.
Yeah, I see Dan pointing out the same reason.
So in V3 you were injecting empty models on purpose via the DI?
Yeah, I find DI useful and necessary, instead of using Facades all over the code.
So i did not mind having an empty Model, because I
$model->find() searched for the record I wanted.Okay, yeah can be if you like that. For models I don't think it is really a facade but it is is a little magic indeed.
I really couldn't imagine a situation where you would want to do that compared to just
Model::create() or whatever tbh, and personally the injection of empty has caused me more headaches at least 5-10 times.Of course I know where the model gets resolved and where not, so I dont have issues with "nullable models".
Yes and no, on surface that looks so but as soon you start doing deep nested relationships for example it already gets way more tricky
I'm not creating the models to be fair. I'm using the dependency to search records.
Can you show an example?
So like
$model->searchForSomething() or so?Let's say I'm rendering a Select component on the "Post" model. The Post model gets injected without isses inside the ->options(callable), but the options i'm trying to render are (for example) living in a "Tags" model. So I inject the "Tags $model" (which is null in this case), but then i go return $model->all()->pluck('id', 'name');
Meaning, I don't expect the model record to be there, because the component is renderd in the "Post" resource, right? I just want the model to be available for me. Same thing happens if you have a service for retrieving all the posts.. you create a Service, inside the constructor you inject the "Post $model" model.. and of course it is null, but you know that and then just get all the posts by calling $model->all().
How would you otherwise solve this..?
Okay, yeah I understand why you're doing it but then I think in the Laravel community you're a bit an outlier that uses
$model->all() instead of just Tags::all()
Could be situations like this (in real life prob more complex but this is a bit what I remember):
Why not just do Select::make('tags')->relationship('tags', 'name') here btw if you are already on a PostResource?Don't get hung up the business logic here, it was just an example. While your suggestion about the relationship makes sense (and would be the right way in this case), there are other closures where I'd like to inject he model and I don't mind it to be null ( or rather, does not contain a resolved record).
I'm using Facades only when I'm really forced too (in static context), while I try to keep injecting things. It's the proper OOP way and while I do understand the convenience from Facades, the codes gets pretty dirty really quick if you get too comfortable with them.
Would you mind explaining how would you tackle a CustomService that has a method which gets all the Posts, for example. And the CustomService implements a interface of "PostRepository" which has a contract to implement: public function all(): Collection;
This is request is purely out of curiosity and somewhat outside if Filament, but this way you'll better understand my logic. I'll provide my implementation as well.
Yeah makes sense, it's probably a different developer mindset. Like, I think everyone sees that Laravel provides for everything one way to do it that can classify as the recommended framework way. Not saying other things are bad, but it's pretty much a fact that for database retrieval Laravel recommends Eloquent and not the repository pattern. My experiences over the years has learned me that generally it is actually long-term the best to stick to all Laravel defaults/recommended way of doing things as much as possible. So that's why I would not probably not have a CustomService or PostRepository at all, but just have 2 Eloquent models Post and Tag and just access them using
Post::all() , Tag::pluck(), etc. I think for a developer it's just so much more convenient to have one Tag model of 10 lines and can do Tag::pluck() immediately compared to having to create a TagRepository interface, the repository and the implementation for each method, as you basically are repeating all the pluck(), all(), cursor(), etc methods from Laravel into your repository classes. So that's a bit where my philosophy comes from.
And I recognize that for some developers it feels like too much magic and not as the theoretically correct way of doing things. But in the end it is us developers maintaining an app and if that goes very elegant and convenient with Eloquent then why would I make the experience worse for myself, the code more complex and less elegant to read just to prevent using magic things. In the end I don't develop with the goal of not using magics as I don't buy anything for that, but I want a clean, elegant and simple codebase that is fun and easy to work in.
One aspect that I am with you on is that magics are not good auto-completion, so you will probably have better and stricter typing than me in some cases. Most issues can be solved with some of the Facade docblocks that Laravel provides and with Laravel Idea, so I don't experience as a very big blocker.Yeah, I understand. I'm not forcing this upon you, so I hope the message is not coming too off putting. I just wanted to know the real reason behind it so I can implement a solution for this since it broke the my application (and have a little discussion on alternatives). While I know that Filament does not directly use Laravel DI (uses EvaluatesClosures on the frontline), It is necessary for me to understand the change which in turn will make my patch functional. I see Dan foresaw this scenario which I've hit and I'm happy that you added the option to allow bound models to still be resolved.
Thanks for putting up with me and taking your time to reply, I really appreciate the effort @ralphjsmit . And I'm glad you defend your position, after all this is open source and we are happy to have people working on projects so others can use it. Cheers 🍻
Thank you too for your openness, definitely not forcing anything upon anyone! I definitely understand your viewpoint as well, though I didn't really expect it when I made the original PR. In your case binding would be great, and you could still use the
app()/resolve() methods manually (though that will probably feel less like a DI scenario for you).If i just add $this->app->bind(Post::class,Post::class) it works again 😉
Awesome, yes definitely :)
Does
$this->app->bind(Post::class) also work (just curious)?It does indeed
Just tested it.
Nice
Okay interesting to know