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

reorganizing the documentation #10

Merged
merged 1 commit into from
Oct 29, 2015
Merged

reorganizing the documentation #10

merged 1 commit into from
Oct 29, 2015

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Oct 23, 2015

@Nyholm
Copy link
Member

Nyholm commented Oct 23, 2015

Awesome!

<?php
require('vendor/autoload.php');

TODO: create client instance with discovery and do some requests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some todo remaining. not sure if i get to work on this much tomorrow. maybe we should merge with the TODOs left in here.

Copy link
Member

Choose a reason for hiding this comment

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

Sure.

@@ -5,56 +5,16 @@ There are some major changes between the two library. This guide will help you u

## New vendor

The new organization name is **PHP HTTP**. This suggests that while our main product is the HTTP Adapter project, we are not limited to it. We plan to collect any kind of HTTP related packages written in PHP.
The new organization name is **PHP HTTP**. This suggests that while our main product is the Httplug project, we are not limited to it. We plan to collect any kind of HTTP related packages written in PHP.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this chapter could be expanded a bit, but not urgent.

Copy link
Member

Choose a reason for hiding this comment

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

Such information should probably go on the website, not the documentation.

Copy link
Contributor Author

@dbu dbu Oct 28, 2015 via email

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Don't know. I usually try to avoid including UPGRADE file in the repository, as there are no BC breaks between major releases, minor releases are usually rare and such information can go into the documentation. We should have something like this in the documentation as well, but the history and description are probably out of scope. We should mention that httplug replaces ivory, and some updating information, done.

Copy link
Contributor Author

@dbu dbu Oct 29, 2015 via email

Choose a reason for hiding this comment

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

@dbu
Copy link
Contributor Author

dbu commented Oct 23, 2015 via email

@Nyholm
Copy link
Member

Nyholm commented Oct 23, 2015

I agree, nothing is ugent. But this documentation helps a lot.
I think we could merge it with todo. As long as they are resolved before a release.

@Nyholm
Copy link
Member

Nyholm commented Oct 23, 2015

It is a great start. The concept of virtual packages are quite unused. You have explained it well but I believe you have to be even more explanatory. But that may be covered in the "how to implement this in a reusable package" section.

@@ -1,87 +1,34 @@
# HTTP Adapter
Copy link
Member

Choose a reason for hiding this comment

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

Change the title to Httplug or Http Client ?

Copy link
Member

Choose a reason for hiding this comment

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

I would go with Httplug. Httpl\Client is used only for namespace.

@sagikazarmark
Copy link
Member

@Nyholm I remember back then I started explaining the rationale behind virtual package architecture, also gave some examples what to require in an application, what to require in a reusable package. Not sure if it is removed, but I think not.


The HTTP Adapter abstracts from PHP HTTP clients that are based on [PSR-7](http://www.php-fig.org/psr/psr-7/).
Httplug abstracts from HTTP clients written in PHP that are based on [PSR-7](http://www.php-fig.org/psr/psr-7/).
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is correct, like we want to provide guzzle 5 adapter where the client is not based on PSR-7.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this wants to be that the abstraction is created based on existing PSR7 clients (which I wonder also if it's true, as the first PSR7 client was guzzle, and we started to work on the interface long before PSR7 was out)

@sagikazarmark
Copy link
Member

@dbu 👍

- Repository: https://github.com/php-http/message-factory
- Documentation: [Message Factory](message-factory.md)

## *-adapter
Copy link
Member

Choose a reason for hiding this comment

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

*-adapter or *-client

Or maybe we should just rename socket-client to socket-adapter (it may be not an adapter to a package, but in a sense it is an adapter for socket / stream function in PHP)

Copy link
Member

Choose a reason for hiding this comment

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

I think we could use both. The basic is the client, since it is a client interface. The next level is an adapter (which is still a client).

@dbu
Copy link
Contributor Author

dbu commented Oct 28, 2015

i added a section specifically for the virtual package topic. also with the idea that people can link to that in their projects.

updated for all the other comments. unless there is some big problem, i would love to merge this and work on the tutorial in follow-up PR and also for moving the upgrading documentation to a file in the code repository.


When *writing a reusable library*, the `require` section should only mention `php-http/client-implementation`, while the `require-dev` section needs to specify a concrete implementation in most cases, in order for the package to be installable on its own for development and testing.

When *using a reusable library* in an application, the `require` section needs to include a concrete implementation in order for the application to install. Missing to do that will lead to composer reporting a missing package.
Copy link
Member

Choose a reason for hiding this comment

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

This line feels strange. It's correct but strange.. Maybe:

When using a reusable library in your application, one must require a concrete implementation. Make sure the require section includes one package that provides php-http/client-implementation. Failing to do that will lead to composer reporting a missing package.

Copy link
Member

Choose a reason for hiding this comment

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

You maybe should include the composer error message. I believe many people are going to search for "The requested package php-http/client-implementation could not be found in any version, there may be a typo in the package name."

$ composer update
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - The requested package php-http/client-implementation could not be found in any version, there may be a typo in the package name.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, it is a bit weird. I would close to the problem from a library point of view: libraries does not depend on an actual implementation, but a virtual one. This is to avoid hard coupling and to allow using an implementation chosen by the user. Applications however must require one providing that virtual package. You can think about this as an "interface/contract" for packages.

I don't think including the composer error message adds any value. This is a composer detail. The actual message doesn't even matter as it leads directly to the fix, so it does not help identifying the problem, it's itself the identification. Enough knowledge to use composer is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think the error message is a very good idea for one reason: people will google that error message and they should find a page that explains what is going wrong. would be awesome if one day we could register virtual packages in packagist and have composer output an explanation, but until that day we should have this

Copy link
Member

Choose a reason for hiding this comment

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

Okay, if something, then a gif animation, because it looks good 😜

For copy pasters we can have the textual error as well.

@joelwurtz
Copy link
Member

👍

1. Usage in a project
2. Usage in a reusable package

In both cases, the client provides a `sendRequest` method to send a PSR-7 `RequestInterface` and returns a PSR-7 `ResponseInterface` or throws an exception that implements `Http\Client\Exception`. The client also has a `sendRequests` to send requests in parallel. Clients that do not support sending requests in parallel will just send them sequentially.
Copy link
Member

Choose a reason for hiding this comment

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

Send requests is not true anymore.

@sagikazarmark
Copy link
Member

@dbu nice work. I added a few comments which we should discuss first, or open separate issues.

@dbu
Copy link
Contributor Author

dbu commented Oct 29, 2015

thanks a lot for the close read. i think there was a lot of clarifcation, great work! i created a bunch of new issues, propose that we merge this now.

@sagikazarmark
Copy link
Member

Go ahead.

dbu added a commit that referenced this pull request Oct 29, 2015
reorganizing the documentation
@dbu dbu merged commit a292396 into master Oct 29, 2015
@dbu dbu deleted the refactor-doc branch October 29, 2015 08:50
@dbu
Copy link
Contributor Author

dbu commented Oct 29, 2015

funny, from working with my team i am used to never click merge on a pull request i created :-)
we are very much into peer reviews for the stuff we do...

This was referenced Oct 29, 2015
@sagikazarmark
Copy link
Member

At work I also follow this principle. In OSS I don't like to be that strict. I review the PR anyway, but devs might want to add something else in the project and they are free to. Also, guys working on OSS are usually sane developers, who require less control to produce sane code 😛

So review is only for mistakes, rarely for sanity check.

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.

Virtual packages
6 participants