-
-
Notifications
You must be signed in to change notification settings - Fork 506
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
2.x - 馃ズ final __constructor
really takes the jelly out of my doughnut
#2813
Comments
final __constructor
really takes the jelly out of my doughnutfinal __constructor
really takes the jelly out of my doughnut
Thanks for taking the time to explain the issues with a final constructor, @mindfullsilence. We definitely hear you. We don鈥檛 want to restrict advanced uses cases like yours in Timber. It should work both for developers with less and advanced experience. But I feel that improving this throughout the Timber codebase is too much for the 2.x release. For the 2.x release, I don鈥檛 mind removing @nlemoine What do you think? |
@mindfullsilence Thanks for your detailed issue and solution proposals. I stumbled upon an issue myself with the I would still add a couple of notes on this.
Not completely, the class constructor isn't the only way to inject services. use Psr\Log\LoggerInterface;
class Thing extends Post {
private LoggerInterface $logger;
public function setLogger(LoggerInterface $logger) {
$this->logger = $logger;
}
} You could also use a The $post = $container->get(Post::class); This is an uncommon way to grab your models. Can you provide a pseudo code real life example? Just so we can see different use cases of |
Really appreciate the consideration and action on this one, guys. Totally understand sticking with a revert for now until more consideration is taken into account down the road. To @nlemoine's point regarding dependency injection through setters, I agree - setters do work. I actually mention this in my OP:
Setters are how I ended up working around my problem, but it feels super messy and brittle compared to asking for the dependencies in the constructor itself. Boiling down a simple use-case right now would be tough. However, shortly after my project is complete (expected completion date is Jan 1st 2024), I'll return here to at-minimum provide some more in-depth use cases for you, and hopefully share with you the theming framework I've been working on (if I can convince my bosses to allow me to open-source, which has a good chance of happening). In essence, it's an orchestrator of Timber, Acorn, and ACF Builder, and I see it as important to the goals of my project to allow the theme developer to utilize the service container and dependency injection capabilities built into Laravel (Acorn provides Laravel functionality to WordPress). One of the major benefits to Laravel's service container is autowiring constructor dependencies without the need to manually register your class with the container. Making that a part of my project is a big goal. |
Thanks for your feedback @mindfullsilence. Now that you provided some details about your stack, I'm guessing your problem lied in things like: $container->make(Timber\Post:class, [
'post' => $post,
]); Just like I mentioned before, DTO are usually not using injected services. This is an extended implementation of the
Nice! Not so much into Acorn but I'm huge ACF Builder user. I'm currently developing a library based on it that's quite promising if I find the time to finish it. |
Sweet! If you ever complete it, I'd be happy to give it a thorough test drive! |
Please don't do this to the end users (me). I'm in the midst of using v2rc1 and came across this restriction - which seriously limits my ability to provide services to the class unless I write a bunch of setter methods or create an init function that I have to call every time the class is instantiated (both options aren't great). This removes any possibility of DI, autowiring, and I'm sure a lot of other things I can't think of right now.
In addition, the logic for this decision seems flawed: the
build()
method also shares the same concern you're discussing in pull 2660 about the constructor - it's just not throwing a warning because PHPStan only looks at the constructor for this potential issue. If I override thebuild()
method with a different signature, it will break just as many things as overriding the__constructor()
would. Marking both thebuild
and__constructor
as final would basically make initializing the class with additional setup code incredibly messy, if not impossible - but it doesn't make sense to mark one without marking the other for this PHPStan warning.How do you deal with this PHPStan warning, then? Simply don't define a constructor in the
Timber\Post
class. The library is already structured exactly how it should be to allow for this. By not defining it, you allow it to be defined by child classes, and the PHPStan warning goes away.To protect against the issue PHPStan is warning about, but for the
build
method, define an Interface based on what PostFactory needs. Also protect yourself from improperly written child classes by moving some of the import logic around and marking theimport()
method asfinal
.Now if the user wants to extend and setup their own constructor dependencies - it's easy, and reliable:
Pretty pretty please with sugar on top, please don't kill this PHPStan mosquito with a
final __constructor()
cannon.The text was updated successfully, but these errors were encountered: