Skip to content
This repository was archived by the owner on May 13, 2023. It is now read-only.

Remove ClientContextFactory #10

Merged
merged 1 commit into from
Nov 16, 2015
Merged

Remove ClientContextFactory #10

merged 1 commit into from
Nov 16, 2015

Conversation

sagikazarmark
Copy link
Member

No description provided.

@sagikazarmark sagikazarmark added this to the v0.2 milestone Nov 12, 2015
@sagikazarmark sagikazarmark changed the title Remove ClientContextFactory, close #7 Remove ClientContextFactory Nov 16, 2015
@mekras
Copy link

mekras commented Nov 16, 2015

I'd like to keep this interface. Actually I've already planned to use it to minimize injected dependency count.

@mekras
Copy link

mekras commented Nov 16, 2015

I changed my mind 😄 I better implement it in my project by myself.

@sagikazarmark
Copy link
Member Author

Actually it violates a few SOLID principles. Also, these factories are implemented on their own (MessageFactory, StreamFactory, etc). How would you implement them in one class? Multiple inheritance is missing from PHP, traits are kind of replacement, but do not provide real solution (implementing factories as traits is not a good idea). If you can provide any viable use case, then I would reconsider removing it, but right now, I don't see the added value of this interface (actually, quite the opposite).

@dbu
Copy link
Contributor

dbu commented Nov 16, 2015

if we see the need for something like this, we better have a concrete class that aggregates the 3 interfaces and forwards the methods. the use case of reducing the number of dependencies seems ok to me - if a class needs factories for all 3 things. but lets remove the interface for now, yes.

sagikazarmark added a commit that referenced this pull request Nov 16, 2015
@sagikazarmark sagikazarmark merged commit 1acc74f into master Nov 16, 2015
@sagikazarmark sagikazarmark deleted the remove_client_context branch November 16, 2015 12:37
@mekras
Copy link

mekras commented Nov 16, 2015

@sagikazarmark, that's why I changed my mind, just couldn't put it into English words.

@sagikazarmark
Copy link
Member Author

Cool. 👍 The important thing is we understood each other at the end. 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants