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

Merge HTTPlug (PHP-HTTP) factories into single class #62

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

Zegnat
Copy link
Collaborator

@Zegnat Zegnat commented Aug 1, 2018

Basically more streamlining. This addresses my own confusion with the existence of separate factory files, as mentioned in #55 (comment).

Based on #60 and should be merged after.

With this change I also noticed we are calling our own factories within methods of the PSR-7 classes… why?

@Zegnat Zegnat requested a review from Nyholm August 1, 2018 10:34
@Nyholm
Copy link
Owner

Nyholm commented Aug 1, 2018

Hm. Moving them to one factory makes sense. I agree. However, that will mess up php-http/discovery. They rely on these classes. We could update the discovery strategy. That would work if we assume everyone updates their dependencies.

What do you think? Is it worth changing these classes?
I think I would rather leave them and deprecate them as soon as 1.0 is released. I know that HTTPlug will deprecate these factories "soon" as well.

@Zegnat
Copy link
Collaborator Author

Zegnat commented Aug 1, 2018

Rebased this now that #60 was merged.

What do you think? Is it worth changing these classes?

On the one hand I would love for the 1.0.0 release to have a minimal amount of files and think it is weird that we can’t do that because of a completely unrelated package (minus your involvement in HTTPlug in general).

On the other hand, if HTTPlug is going to pull the rug from php-http/message-factory anyway, maybe we should cut our losses and look forward to a 2.0.0 release completely without these classes instead?

* @author Tobias Nyholm <tobias.nyholm@gmail.com>
* @author Martijn van der Ven <martijn@vanderven.se>
*/
final class PhpHttpFactory implements \Http\Message\MessageFactory, \Http\Message\StreamFactory, \Http\Message\UriFactory
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets name it to HttplugFactory

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Zegnat
Copy link
Collaborator Author

Zegnat commented Aug 1, 2018

Blocked by #64, Request and Response currently depend on a factory. Rather than renaming the factory inside those classes again, lets wait for that to be unnecessary and then rebase.

@Zegnat
Copy link
Collaborator Author

Zegnat commented Aug 2, 2018

Squashed and rebased on new master. Waiting for checks to complete. Anything else blocking this?

Copy link
Owner

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@Nyholm Nyholm merged commit c0ac45f into master Aug 2, 2018
@Nyholm Nyholm deleted the phphttp-factory branch August 2, 2018 11:26
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.

None yet

2 participants