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

[#2138]enhance the function for the injector (Add the injectable interface) #1186

Merged
merged 3 commits into from
Oct 13, 2017

Conversation

zhaoyin
Copy link
Contributor

@zhaoyin zhaoyin commented Sep 4, 2017

Hi,the Injector inject beans in controllers,now I enhance the function of the injector

#Lighthouse 2138

@zhaoyin zhaoyin changed the title * enhance the function of the injector (Add the injectable interface) * enhance the function for the injector (Add the injectable interface) Sep 4, 2017
@zhaoyin zhaoyin changed the title * enhance the function for the injector (Add the injectable interface) [#2138]enhance the function for the injector (Add the injectable interface) Sep 6, 2017
@asolntsev asolntsev self-assigned this Sep 18, 2017
@asolntsev
Copy link
Contributor

@zhaoyin Thank you!
This is basically a good change, except two nuances.

  1. I guess "Injectable" is a wrong term. "Injectable" means the thing being injected.
    What you need is "InjectionClient" or probably "RequiresInjection" (see https://en.wikipedia.org/wiki/Dependency_injection).
  2. I suggest to create an annotation instead of interface. It seems to be a better design:
@RequiresInjection
public class MyJob extends Job {
  @Inject MyService service;
}

@zhaoyin
Copy link
Contributor Author

zhaoyin commented Sep 20, 2017

hi,@asolntsev,I know that,but in playframework,the Injector class in inject package,the original method public static void inject(BeanSource source) inject beans in controllers that implements ControllerSupport,ControllerSupport is a interface ,but i support controllers only,so i use the feature ,so i use the injectable interface instead of an annotation to the minimal changes.it can support all classes if the class implement the injectable,i think it's ok,and you ?

@zhaoyin
Copy link
Contributor Author

zhaoyin commented Sep 20, 2017

and when ever was the milestone 1.5 release publishing ? thanks!

@asolntsev
Copy link
Contributor

@zhaoyin I agree that "minimal changes" is a valid concern.
But in this case, using annotation is also very simple:

classes.addAll(Play.classloader.getAnnotatedClasses(RequiresInjection.class));

Byt the way, why do you want to inject dependencies somewhere but controllers and jobs?
Typically it's not needed. The main entry point is controller. Controller injects its dependencies, and every of its dependencies injects its own dependencies, and so on. No need for any custom injection.

@zhaoyin
Copy link
Contributor Author

zhaoyin commented Sep 21, 2017

@asolntsev ,many projects have Layered development,every layer service should be a interface for it's upper layer but only for controller,especially the project got bigger than before.the some layer of the project may be a part of other project.so i think the injector shouldn't for the controller...And you?

@asolntsev
Copy link
Contributor

@xael-fry I recommend to merge this PR.

@asolntsev asolntsev added this to the 1.5.1 milestone Oct 13, 2017
@asolntsev asolntsev merged commit 5239321 into playframework:master Oct 13, 2017
@asolntsev
Copy link
Contributor

@zhaoyin Please also review my next pull request: #1198

Probably it will also be useful for you?

@asolntsev
Copy link
Contributor

@zhaoyin Please answer: how exactly do you use play.inject.Inject class?
As I see from Play source code, this class is never used.

@zhaoyin
Copy link
Contributor Author

zhaoyin commented Nov 29, 2017

defined a plugin for Inject(e.x. Spring,Guice),in the plugin ,I use the method public static void inject(BeanSource source) and the annotation play.inject.Inject

@asolntsev
Copy link
Contributor

@zhaoyin Thank you for the answer. We also use play-guice plugin.

Then I don't understand why you (and me) need the class play.inject.Inject. Why can't you just do the same thing in your plugin?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants