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

Nette extension: do not autowire template helper #64

Merged
merged 1 commit into from
Jun 11, 2015
Merged

Nette extension: do not autowire template helper #64

merged 1 commit into from
Jun 11, 2015

Conversation

lookyman
Copy link
Contributor

..also previous setInject was pointless.

@florianeckerstorfer
Copy link
Member

I have no experience with Nette what-so-ever, would you mind to explain this change to me and does it break existing usage of the service?

@lookyman
Copy link
Contributor Author

Technically yes, it is a BC break. Previously, the template helper was registered as an autowireable service in DI container, which meant you could autowire it by it's class name (like, in typehint) into your services if you wanted to. Now, if you want to achieve the same result, you have to pass it manually in config by it's service name (@slugify.helper).

However, the only sane place in which you want to have a template helper is Latte engine (which creates templates for you), and the extension does that already. If you want to have a Slugify service anywhere else, you should autowire SlugifyInterface instead. This is the best practice and all other similar helpers in Nette or other extensions behave this way. It was also my original intention when I wrote this extension, so the current state is actually a mistake.

The setInject(false) thing was pointless because it turns off some functionality for this service that it doesn't have.

@florianeckerstorfer
Copy link
Member

Thanks for taking the time explaining this. I will just trust you on this.

florianeckerstorfer pushed a commit that referenced this pull request Jun 11, 2015
Do not autowire template helper in Nette integration
@florianeckerstorfer florianeckerstorfer merged commit cdd2426 into cocur:master Jun 11, 2015
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.

2 participants