Skip to content

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

Merged
dg merged 1 commit into
nette:masterfrom
matej21:feature/compiler_prefix
Apr 29, 2016
Merged

Compiler::parseServices: allow prefixing service name in the config#85
dg merged 1 commit into
nette:masterfrom
matej21:feature/compiler_prefix

Conversation

@matej21

@matej21 matej21 commented Sep 13, 2015

Copy link
Copy Markdown
Contributor

@greeny

greeny commented Sep 13, 2015

Copy link
Copy Markdown
Contributor

Nice one! 👍

@fprochazka

Copy link
Copy Markdown
Contributor

Nice!

@dg

dg commented Oct 9, 2015

Copy link
Copy Markdown
Member

What about something like

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

@matej21

matej21 commented Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

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

@dg

dg commented Oct 9, 2015

Copy link
Copy Markdown
Member

Hmmm, passing service names to methods seems like antipattern.

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

matej21 commented Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

updated

@dg

dg commented Oct 9, 2015

Copy link
Copy Markdown
Member

Great!

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

@matej21

matej21 commented Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

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

greeny commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

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

dg commented Oct 13, 2015

Copy link
Copy Markdown
Member

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 commented Jan 13, 2016

Copy link
Copy Markdown
Member

@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.

@milo milo added this to the v2.4 milestone Mar 29, 2016
@dg dg force-pushed the master branch 2 times, most recently from 231a29c to 7f12a9f Compare April 21, 2016 13:03
@matej21 matej21 force-pushed the feature/compiler_prefix branch from b450498 to c2b87de Compare April 29, 2016 18:53
Comment thread src/DI/Compiler.php Outdated
} elseif ($namespace) {
$name = $namespace . '.' . $name;
}
if ($namespace) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move it bellow next condition?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@matej21 matej21 force-pushed the feature/compiler_prefix branch from c2b87de to 2a2be47 Compare April 29, 2016 19:00
@dg

dg commented Apr 29, 2016

Copy link
Copy Markdown
Member

Thanks!

@dg dg merged commit 5159e83 into nette:master Apr 29, 2016
@matej21

matej21 commented Apr 29, 2016

Copy link
Copy Markdown
Contributor Author

great! now I can finally fix the doc :)

dg pushed a commit that referenced this pull request Apr 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants