Skip to content
This repository has been archived by the owner on Feb 6, 2020. It is now read-only.

Add setAliases and setFactories methods branch 2.7 #115

Closed
wants to merge 3 commits into from

Conversation

malinink
Copy link
Contributor

@malinink malinink commented Apr 6, 2016

We should add methods to branch 2.7 in order to change other components that to use them before we migrate to SM3.

More information could be found here #112

All components listed in symmetric PR #114

@Ocramius
Copy link
Member

Ocramius commented Jun 1, 2016

Disagree with this approach. This API should at least be private and used only by internal zendframework/zend-servicemanager components. public is a no-go in my opinion, especially after all the effort put into reducing the surface of interaction with the ServiceManager

@malinink
Copy link
Contributor Author

malinink commented Jun 4, 2016

@Ocramius
If that methods will be protected or private then we could not use them outside class.
Both of methods made for the same as a function setFactory and setAlias (and that method are public).

The reason of creation new methods is new SM3, where it is better to add all aliases or factories at once due to performance issues.

Maybe there are any other ways to do that? (all discussion is here #112 and here #122)

@Ocramius
Copy link
Member

Ocramius commented Jun 5, 2016

The reason of creation new methods is new SM3, where it is better to add all aliases or factories at once due to performance issues.

I understand that. I wonder if we have a way to avoid that, and instead merge configs upfront (which would remove an entire set of problems in this space completely)

@weierophinney
Copy link
Member

Use configure() is you want to do multiple aliases or factories at once.
It's not BC, bit works for v3.
On Jun 5, 2016 3:43 PM, "Marco Pivetta" notifications@github.com wrote:

The reason of creation new methods is new SM3, where it is better to add
all aliases or factories at once due to performance issues.

I understand that. I wonder if we have a way to avoid that, and instead
merge configs upfront (which would remove an entire set of problems in this
space completely)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#115 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABlVyAivURX1pWBgsXDjQ4OGNjBiC9yks5qIzUCgaJpZM4IBZRY
.

@malinink
Copy link
Contributor Author

malinink commented Jun 6, 2016

@Ocramius @weierophinney
As I see, these PR already exist in all depend components.

So it is better to close both my PR (#114 #115), and issue (#112).

@Ocramius
Copy link
Member

Ocramius commented Jun 6, 2016

@Ocramius Ocramius closed this Jun 6, 2016
@Ocramius Ocramius self-assigned this Jun 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants