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

Use DI container to load global middleware classes #94

Merged
merged 2 commits into from
Dec 6, 2021

Conversation

clue
Copy link
Owner

@clue clue commented Dec 3, 2021

This changeset makes sure we use the same DI container instance to also load global middleware classes. This allows us to take advantage of autowiring for global middleware classes the same way it can be done for route request handlers as implemented in #92. This does not otherwise break existing APIs, so this is a pure feature addition.

Builds on top of #92 and #89

@clue clue added the new feature New feature or request label Dec 3, 2021
@clue clue requested a review from SimonFrings December 3, 2021 17:20
@HLeithner
Copy link
Contributor

What's the benefit not directly use the PSR container interface?

@clue
Copy link
Owner Author

clue commented Dec 4, 2021

@HLeithner One step at a time 👍 We love standards and we'll definitely add PSR-11 support in a future PR 💯

We've already added an outlook on a future integration with #92 here: https://framework-x.org/docs/best-practices/controllers/#container-configuration

@HLeithner
Copy link
Contributor

I have seen it but I'm not sure what's the benefit to create an own interface that you want to replace or glue to the PSR-11 afterwards but maybe I don't see the complete picture yet.

@clue
Copy link
Owner Author

clue commented Dec 4, 2021

@HLeithner Very good points, there are of course pros and cons to everything!

We've tried to lay out our philosophy in the documentation (https://framework-x.org/docs/getting-started/philosophy/), in particular:

  • make easy things easy & hard things possible
  • Batteries included, but swappable
  • Reuse where applicable, but accept some duplication

In the current version, the Container class is entirely internal (see @internal annotation), this targets points 1 and 2. In a future version, you'll be able to swap it out using your own container, this targets points 1 to 3.

You're right that it's hard to see the full picture with the current PR. Let's continue this discussion in the follow-up PR when the container API will be made public?

I very much appreciate your input and we're actively looking into ways to get the community more involved. Let's set up a discussion thread to see if another meetup etc. would be an idea? 👍

@HLeithner
Copy link
Contributor

Ok thanks for the answer and it makes sense based on the philosophy.

Sure meetup is always welcome.

@SimonFrings SimonFrings merged commit 34d379d into clue:main Dec 6, 2021
@clue clue deleted the container-class branch December 6, 2021 09:25
@SimonFrings SimonFrings added this to the v0.6.0 milestone Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants