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

2.3 bc breaks #53

Closed
hrach opened this issue Feb 11, 2015 · 6 comments
Closed

2.3 bc breaks #53

hrach opened this issue Feb 11, 2015 · 6 comments

Comments

@hrach
Copy link
Contributor

hrach commented Feb 11, 2015

Can't believe that nobody has complained yet. I'm trying to run my OrmExtension. I'm not sure if I understand the problem correctly, but I think there are 2 issues/BC breaks.:

https://github.com/nextras/orm/blob/fe20359d7b557d83157d4a71d610e21a36c574f1/src/Bridge/NetteDI/OrmExtension.php

  • Inject extension is basically added too early, so it can inject services, which are not there yet. I'm adding services in beforeCompile method. Maybe I'm doing it wrong, but loadConfiguration method doesn't seem to be the right one for adding services.
  • Inject extension works without resolved class names, so even if I add InjectionExtension after the OrmExtension, it doesn't work, because one more $this->builder->prepareClassList(); call is needed.
@lookyman
Copy link
Contributor

You should never add any services after loadConfiguration, even if you have to partially configure them later!

http://doc.nette.org/en/2.2/di-extensions#toc-beforecompile

#46 (comment)

@hrach
Copy link
Contributor Author

hrach commented Feb 11, 2015

@lookyman well, I understand that this is the only way how to get this working now, but it's massive BC break, nowhere mentioned; also the API & the method name is bad, it makes me really unhappy to create service definitions in method named loadConfiguration.

There are two possibilities:

  1. Make it (somehow) backward compatible.
  2. Let this be, and disallow adding new services in beforeCompile time, to fail asap.

But, to be honest, the motivation to call prepareClassList() (added after my feature request), is exactly the possibility to add services dynamicly in beforeCompile time. Otherwise, I don't see much added value.

@xificurk
Copy link
Contributor

it's massive BC break, nowhere mentioned

http://doc.nette.org/en/2.2/di-extensions#toc-beforecompile

In beforeCompile phase we should not add any more services, however you can modify already existing ones or add relations between services (for example using tags).

You're doin' it wrong ;-)

@dg
Copy link
Member

dg commented Feb 11, 2015

it makes me really unhappy to create service definitions in method named loadConfiguration.

Service definitions are configuration of DIC. So it was meant as "load your configuration into container builder", not "load configuration from some file or whatever".

@dg dg closed this as completed in 4c0c3b3 Feb 11, 2015
@enumag
Copy link
Contributor

enumag commented Feb 12, 2015

@hrach There is actually much more added value by the early preparedClassList. Mainly Kdyby/Events don't work well without it and now we can use $builder->getByType() in beforeCompile which allows you to not relly on service names of other extensions. There were some other cases where it helped me which I no longer remember.

@hrach
Copy link
Contributor Author

hrach commented Feb 14, 2015

@dg Thanks! What about deprecating adding new service in beforeCompile()?

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

No branches or pull requests

5 participants