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

Allow overwriting an Alias in an extending class #117

Merged
merged 2 commits into from
Mar 18, 2018

Conversation

heiglandreas
Copy link
Member

Currently Disco throws an exception when it finds an already declared alias. That is totally valid as long as the configuration is done in one single file.

As soon as you want to have a configuration split over different files that becomes complicated as you can not overwrite an alias that was declared in the base class within an extending class.

Given I want a configuration for my production system with an alias for a logger and I then want to extend that configuration for my dev-environment and want to implement a different logger.

Currently I need to create three classes. One base class with all the configs that will never change and one class for production config extending the base class and implementing all the aliases that will change between prod and dev. And I also create another config for the dev-counterpart of those changing aliases.

This commit allows to overwrite aliases in extending files only. So I can create my configuration for production and for dev I can extend that base class and overwrite the aliases I need to adapt.

This change should be absolute BC, no tests where adapted, only added.

Currently Disco throws an exception when it finds an already declared
alias. That is totally valid as long as the configuration is done in one
single file.

As soon as you want to have a configuration split over different files
that becomes complicated as you can not overwrite an alias that was
declared in the base class within an extending class.

Given I want a configuration for my production system with an alias for
a logger and I then want to extend that configuration for my
dev-environment and want to implement a different logger.

Currently I need to create three classes. One base class with all the
configs that will never change and one class for production config
extending the base class and implementing all the aliases that will
change between prod and dev. And I also create another config for the
dev-counterpart of those changing aliases.

This commit allows to overwrite aliases in extending files only. So I
can create my configuration for production and for dev I can extend that
base class and overwrite the aliases I need to adapt.

This change should be absolute BC, no tests where adapted, only added.
@heiglandreas
Copy link
Member Author

heiglandreas commented Mar 12, 2018

Have a look at this gist for more details 😉

@shochdoerfer
Copy link
Member

"Normally" I would tell you to overwrite the method in the child class and not make use of the aliases feature. But given you use Expressive you have to use the aliases due to the classnames as identifier way of naming things. Could you check if naming both methods the same would work for you? In theory it should, but not sure in practice.

Not sure if I like the rule of aliases being unique but you can overwrite them in child classes. Maybe we could allow aliases to be overwritten if the parent alias is flagged in a certain way.

@heiglandreas
Copy link
Member Author

heiglandreas commented Mar 12, 2018

Well… I didn't want to introduce a new Annotation or flag 😉

@heiglandreas
Copy link
Member Author

And actually that doesn't work in that specific case as the return-type differs. Therefore it's not an overwritten method but a completely new method that needs to be linked to the same alias… 😦

@shochdoerfer
Copy link
Member

shochdoerfer commented Mar 12, 2018

Given you want to replace dependencies the return type should be a common interface or base class. Otherwise you would not be able to swap things out. So that should not be a show-stopper for me.

On the other hand, overriding aliases in sub classes would behave like overriding methods in sub classes, one could say that this is logic then.

@heiglandreas
Copy link
Member Author

I was also thinking along the lines of a common interface only to find that Zend\Expressive doesn'T behave that way always. In that actual example I want to create an ErrorResponseGenerator only to find out that the implementation of Zend\Expressive\Middleware\ErrorResponseGenerator does neither extend nor implement anything. Equally the development implementation of the Zend\Expressive\Middleware\WhoopsErrorResponseGenerator 😦

Both have a common `__invoke-method and act as Middleware, but they don't implement anything that defines them as such…

So 💩 like that can happen and I'm not sure I'd want to swap out my DI-Container just because someone f***ed up their library… 😉

@shochdoerfer shochdoerfer merged commit 7241bc8 into bitExpert:master Mar 18, 2018
@shochdoerfer shochdoerfer added this to the 0.10.0 milestone Mar 18, 2018
@shochdoerfer
Copy link
Member

Thank you for your contribution!

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