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

Upgraded CallableResolver to Advanced interface #71

Merged
merged 3 commits into from
Nov 1, 2021

Conversation

Rarst
Copy link
Contributor

@Rarst Rarst commented Apr 29, 2021

  1. Adds support for Slim callable notation with single colon (name:method).
  2. Enables resolving PSR objects, so that addressing them by class name isn't broken.

Fixes #51

Alternative to #70, I tried to not change constructor signature. Since interface extends the previously used one, backwards compatibility should be intact.

One implementation difference with upstream is that resolver there depends directly on container and can do container lookups. I would prefer to do that, but backwards compatibility issue, same as above.

Related disparity with Slim is closure callbacks not binding to container, see #52.

Includes unit tests for everything I could think of, but needs more people to look at. A lot of possible cases between all the syntaxes and contexts.

@shadowhand
Copy link

Love this. I would like to see types added, but otherwise this is 💯 !

@Rarst
Copy link
Contributor Author

Rarst commented Apr 29, 2021

I would like to see types added

Where do you think types could be improved? :) The inputs/outputs are very mixed...

@shadowhand
Copy link

shadowhand commented Apr 29, 2021

Where do you think types could be improved? :) The inputs/outputs are very mixed...

Ah, I see that the typing is largely controlled by the interface, and that there are callable|string|array types, which is annoying. Seems this is as good as it can get. 👍

@mnapoli
Copy link
Member

mnapoli commented Apr 30, 2021

@Rarst that looks great, thanks a lot!

Related disparity with Slim is closure callbacks not binding to container, see #52.

I think this is a very good thing 👍 no problem here.

@Rarst would you be interested that I add you as maintainer to this project? This is a non-trivial change, and I want to avoid being a blocker on any fix or other change like this in the future. There is no commitment, it's mostly to give you a bit more room.

@Rarst
Copy link
Contributor Author

Rarst commented Apr 30, 2021

would you be interested that I add you as maintainer to this project?

I don't mind, but I tend to hop around, that is things have most of my attention when I am actively messing with them for my needs. :)

@mnapoli
Copy link
Member

mnapoli commented Nov 1, 2021

Very sorry for the delay, thanks a lot for all the work!

🚀

@mnapoli mnapoli merged commit 1644a2f into PHP-DI:master Nov 1, 2021
@Rarst Rarst deleted the advanced-callable-resolver branch November 1, 2021 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom implementation of CallableResolver breaks deferred middleware
3 participants