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

Allow injection of any container object as factory parameter via type hinting #333

Merged
merged 3 commits into from Nov 29, 2015

Conversation

@jdreesen
Copy link
Contributor

@jdreesen jdreesen commented Sep 21, 2015

With this PR it's possible to inject any container object into a factory via type hinting (see #197).

This means that instead of fetching the needed objects from the container inside your factory like this

factory(function(ContainerInterface $c) {
  return new Thing($c->get(Service::class));
});

you can now let the container inject your dependencies as arguments to your factory directly

factory(function(Service $s) {
  return new Thing($s);
});

This works with multiple arguments, too, of course. And it's backwards compatible, so if you don't type hint the first parameter, an instance of the ContainerInterface will be injected (but that's not recommended anyway).

There was a discussion about "angular style DI" in #197 and #239 but as I wasn't sure what's the best way to define this type of dependencies this feature was left out here.

I did a quick benchmark (my first one using blackfire) with the provided blackfire run --samples=100 php factory.php which showed a performance decrease of about 10% but I'm not sure how reliable this is for real world situations. Maybe you have better ways for benchmarking this?

Todo:

  • documentation
  • changelog
@jdreesen
Copy link
Contributor Author

@jdreesen jdreesen commented Sep 22, 2015

Added documentation and entry in the changelog.

Loading

@mnapoli
Copy link
Member

@mnapoli mnapoli commented Sep 23, 2015

Looks absolutely awesome! I need to test regarding performances too (I have a half-done repository with a few performance tests, I need to finish it a push it sometime soon) but if it's only 10% that seems very good considering how useful it will be.

I'll probably can't review and merge this week, but hopefully I'll have time to look at it soon :)

Loading

@mnapoli mnapoli added this to the 5.2 milestone Sep 23, 2015
@mnapoli
Copy link
Member

@mnapoli mnapoli commented Sep 30, 2015

Hi, just an update: I moved house and don't have internet access yet, it will be like that for another week, sorry for the delay ;)

Loading

@mnapoli
Copy link
Member

@mnapoli mnapoli commented Nov 1, 2015

Getting back to this, sorry for the delay. I tried it out today performance-wise. Form the very simple benchmark it shows a 16% slowdown when using factory(): https://blackfire.io/profiles/compare/974a4ada-38aa-472f-97bd-bf5c3220c1bf/graph And that's just the total time: for the FactoryDefinitionResolver, the time spent there is up by 40%.

It's bother me a little bit even though I really want this feature in. Using closures as factories is a main feature, and when you compare it straight to (for example) Pimple it will be much slower. Maybe there is a little bit of room for improvements to find.

One thing I was looking into lately is caching reflection almost entirely. It's a bit of work and I haven't pushed it on GitHub yet, if you are interested to have a look let me know. Also I have no guarantee at all that it will bring improvements.

Loading

@jdreesen
Copy link
Contributor Author

@jdreesen jdreesen commented Nov 1, 2015

I understand your concerns. Your reflection caching work sounds interesting. Maybe we should wait with this PR until it moved further.

Loading

@mnapoli
Copy link
Member

@mnapoli mnapoli commented Nov 29, 2015

FYI this PR is included in #347, ready to be merged in master for 5.2 (if you have any comment on #347 feel free)

Loading

@mnapoli mnapoli merged commit 71e368d into PHP-DI:master Nov 29, 2015
4 checks passed
Loading
@jdreesen jdreesen deleted the #197 branch Nov 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants