Kdyby / Console

Symfony Console integration for Kdyby components
https://packagist.org/packages/kdyby/console
Other
52 stars 54 forks source link

Fixed DI extension #26

Closed enumag closed 9 years ago

enumag commented 9 years ago

1) You shouldn't rely on some fixed name of the service, it's better to find the services you need using $builder->getByType($class). 2) You should manipulate with services from other extensions in beforeCompile only. First reason is that $builder->getByType($class) doesn't work in loadConfiguration, second is to avoid problems if your extension is registered before the other extension.

enumag commented 9 years ago

Hmm travis is failing for some reason, I'll investigate it.

enumag commented 9 years ago

The reason is probably that prepareClassList is not called soon enough in Nette 2.2 and consequently $builder->getByType($class) doesn't work even in beforeCompile.

enumag commented 9 years ago

I fixed the compatibility with Nette 2.2 by a fallback to the previous behaviour. That means the issue is not fixed for Nette 2.2. It is not a major issue so I don't think it's worth the effort trying to solve it. Is this solution good enough?

fprochazka commented 9 years ago

1) You shouldn't rely on some fixed name of the service, it's better to find the services you need using $builder->getByType($class).

Agreed, but as you might have noticed, this extension is really old and at the time this was written, it was completely valid practice.


Very nice refactoring! :) I'll look thoroughly into this later as it's not critical and I'm a bit bussy right now.

enumag commented 9 years ago

Agreed, but as you might have noticed, this extension is really old and at the time this was written, it was completely valid practice.

Of course, I've used it myself in the past. :-)


It would be better to have a test for this but that means to set up separate composer.json, right?

Another note is that the same refactoring should be done in Kdyby/Translation and maybe some others. I'll look into it after this is merged.

enumag commented 9 years ago

Similar refactoring should be done for the following Kdyby packages as well:

enumag commented 9 years ago

ping @fprochazka

fprochazka commented 9 years ago

Thank you, merged.

coveralls commented 8 years ago

Coverage Status

Changes Unknown when pulling bf1db788250ac1610df80ce623862df1baf0dd0d on enumag:nette-2.3 into \ on Kdyby:master**.