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

Compiler::parseServices: allow prefixing service name in the config #85

Merged
merged 1 commit into from Apr 29, 2016

Conversation

@matej21
Copy link
Contributor

matej21 commented Sep 13, 2015

@greeny

This comment has been minimized.

Copy link
Contributor

greeny commented Sep 13, 2015

Nice one! 👍

@fprochazka

This comment has been minimized.

Copy link
Contributor

fprochazka commented Sep 14, 2015

Nice!

@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 9, 2015

What about something like

services:
    articles: MyBlog\ArticlesModel(@connection)
    comments: MyBlog\CommentsModel(@connection, @self.articles)
@matej21

This comment has been minimized.

Copy link
Contributor Author

matej21 commented Oct 9, 2015

@dg maybe it looks a bit better, but this won't work. But it is probably useless anyway.

@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 9, 2015

Hmmm, passing service names to methods seems like antipattern.

@matej21 matej21 force-pushed the matej21:feature/compiler_prefix branch 3 times, most recently from 7b4d0ec to b450498 Oct 9, 2015
@matej21

This comment has been minimized.

Copy link
Contributor Author

matej21 commented Oct 9, 2015

updated

@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 9, 2015

Great!

I'm just not quite sure that self is the best name for this prefix.

@matej21

This comment has been minimized.

Copy link
Contributor Author

matej21 commented Oct 9, 2015

I'm not sure either since it is used for self-referencing service.

btw, maybe this could be moved to the ContainerBuilder so you can use @self.foo syntax in the CompilerExtension as well (instead of $this->prefix('@foo'))

@greeny

This comment has been minimized.

Copy link
Contributor

greeny commented Oct 13, 2015

btw, maybe this could be moved to the ContainerBuilder so you can use @self.foo syntax in the CompilerExtension as well (instead of $this->prefix('@foo'))

Good idea 👍

What about @this, @current or @scope?

@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 13, 2015

btw, maybe this could be moved to the ContainerBuilder so you can use @self.foo syntax in the CompilerExtension as well (instead of $this->prefix('@foo'))

I can imagine that it could cause complications, for example when somebody will addSetup('@self ...') to service from a different scope. In CompilerExtension you need prefix() only in addDefinition().

@dg dg force-pushed the nette:master branch from 333d42f to 9b0f815 Nov 13, 2015
@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 13, 2016

@current, @this and @self sound similary. @scope sound fine, but has a different meaning in DI http://www.javaranch.com/journal/2008/10/dependency-injection-what-is-scope.html.

@dg dg force-pushed the nette:master branch from ba7b241 to fb96e4f Feb 8, 2016
@dg dg force-pushed the nette:master branch 2 times, most recently from 231a29c to 7f12a9f Apr 21, 2016
@matej21 matej21 force-pushed the matej21:feature/compiler_prefix branch from b450498 to c2b87de Apr 29, 2016
@@ -250,6 +250,9 @@ public static function parseServices(ContainerBuilder $builder, array $config, $
} elseif ($namespace) {
$name = $namespace . '.' . $name;
}
if ($namespace) {

This comment has been minimized.

Copy link
@dg

dg Apr 29, 2016

Member

Can you move it bellow next condition?

This comment has been minimized.

Copy link
@matej21

matej21 Apr 29, 2016

Author Contributor

done

…ices from a same extension
@matej21 matej21 force-pushed the matej21:feature/compiler_prefix branch from c2b87de to 2a2be47 Apr 29, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented Apr 29, 2016

Thanks!

@dg dg merged commit 5159e83 into nette:master Apr 29, 2016
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@matej21

This comment has been minimized.

Copy link
Contributor Author

matej21 commented Apr 29, 2016

great! now I can finally fix the doc :)

dg added a commit that referenced this pull request Apr 29, 2016
…ices from a same extension (#85)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.