Update Composer install section in README.md
#788
Conversation
phansys
commented
Mar 18, 2016
phansys
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | |
| License | MIT |
| Doc PR |
|
Hi @phansys, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution! TTYL, AZPRBOT; |
|
Can one of the admins verify this patch? |
|
|
||
| 1. Create a file named **composer.json** in the root of your project and add the following code to it: |
There was a problem hiding this comment.
The existing instructions will install the dev-master by default with no errors. It is intuitive for new users to follow. I don't see any compelling need to change this.
yaqi
There was a problem hiding this comment.
You can point any branch or released (tagged) version you want at the constraint, but the preferred way to create a local copy with composer is what I'm proposing; since it is more concise and takes advantage of the command provided for this kind of situations/scenarios.
There was a problem hiding this comment.
For beginners, the original instructions are more straightforward. For experienced users, they most likely already know how to handle this. But of course we can have your way as a concise alternative. Can you leave the old instructions and add the new instructions bellow it and mark it such as "Or you can also do this ... "
Thanks
yaqi
There was a problem hiding this comment.
I understand your point @yaqiyang, but there is a main difference between the current way and what I'm proposing: the current way is explaining how to create a dependency for this package in an external project, while what I'm proposing is a way to achieve the same as explained at "Download Source Code" section with Composer.
You can read more about this feature at https://getcomposer.org/doc/03-cli.md#create-project, and obviously, you're aimed to execute the command by yourself and check what is the result.
Please, let me know if you have any additional concerns about it.
There was a problem hiding this comment.
There were 2 problems when I ran you command.
First, I needed to add .phar to composer. The command that works for me was "php composer.phar create-project microsoft/windowsazure ./test ^0.4". Once it worked, it pulled down an older build that did not work because of missing dependencies.
Second, once I ran "php composer.phar create-project microsoft/windowsazure ./test2 dev-master", it pulled down the dev-master, but WindowsAzure folder is not part of the vendor folder. I don't think that is what we want the end users to see, especially for beginners.
The readme file is mostly for beginners and we want to make it very simple to follow.
Thanks a lot for your contribution.
Yaqi
There was a problem hiding this comment.
In your example, the constraint dev-master can be replaced by ^0.4@dev and it is the preferred way in order to respect semver, since it now is an alias for dev-master because #769 is already merged.
Take into account that the result of this command is the same of git clone + composer install.
There was a problem hiding this comment.
Short recap:
composer create-project microsoft/windowsazure /path/to/install/dir ^0.4@dev
will install the standalone package and its dependencies, just like git clone https://github.com/Azure/azure-sdk-for-php.git /path/to/install/dir && composer install
Obviously, that is useful when the user wants to execute test or needs to check some features of the library outside any other project.
On the other hand, the removed lines aims to users to manually edit its composer.json file. Even for beginners it should be a odd case, since if they are trying to use Composer as dependency manager, they should have other declarations specific to its own project, like the name, any other dependencies or autoload mechanisms.
In that case, a better explanation could be:
In order to install this library with Composer as a dependency of your project, you must run the following command:
`php composer require microsoft/windowsazure:^0.4@dev`
which will add the following requirement to your `composer.json` file:
```json
{
"require": {
"microsoft/windowsazure": "^0.4@dev"
}
}Please, note that this command will create a composer.json file if it doesn't exist previously, so there are no issues in both situations.
|
Can you update this change? I think it is ok to use it as an alternative, in addition to the .json file method. Since the .json is very simple now, this command is basically equivalent to the file. |
phansys
force-pushed
the
readme
branch
2 times, most recently
from
9181c18
to
e839064
Compare
|
@yaqiyang, updated. |
|
|
||
| php composer.phar install | ||
| php composer create-project microsoft/windowsazure /path/to/install/dir ^0.4 |
There was a problem hiding this comment.
Do you need to have .phar here?
There was a problem hiding this comment.
Did you try your command? php composer.phar create-project microsoft/windowsazure ./test2 ^0.4 will try to install v0.4.0, not v0.4.2.
php composer.phar create-project microsoft/windowsazure ./test3 ^0.4.2 does not work either
There was a problem hiding this comment.
If there are so many hoops to jump, this will not work for beginners which is the Readme file for.
There was a problem hiding this comment.
@yaqiyang, I'll try to address your comments one by one:
- as I have composer installed globally and my executable is called
composer, I don't need to suffix the call with.phar:
$ which composer
/usr/local/bin/composer
Obviously, it depends on which mechanism the user chooses based on the install documentation: https://getcomposer.org/doc/00-intro.md
- I have no issues installing
0.4.2version with this command:
$ composer create-project microsoft/windowsazure testing-resolved-version ^0.4
You are running composer with xdebug enabled. This has a major impact on runtime performance. See https://getcomposer.org/xdebug
Installing microsoft/windowsazure (v0.4.2)
- Installing microsoft/windowsazure (v0.4.2)
Loading from cache
Created project in testing-resolved-version
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
- Installing firebase/php-jwt (v3.0.0)
Loading from cache
- Installing pear/pear_exception (v1.0.0)
Loading from cache
- Installing pear/net_url2 (v2.2.0)
Loading from cache
- Installing pear/http_request2 (v2.3.0)
Loading from cache
- Installing pear/console_getopt (v1.4.1)
Loading from cache
- Installing pear/pear-core-minimal (v1.10.1)
Loading from cache
- Installing pear/mail_mime (1.10.0)
Loading from cache
- Installing pear/mail_mime-decode (1.5.5.2)
Loading from cache
- Installing mikey179/vfsstream (v1.6.2)
Loading from cache
- Installing phpdocumentor/reflection-docblock (2.0.4)
Downloading: 100%
- Installing phpunit/php-token-stream (1.4.8)
Downloading: 100%
- Installing symfony/yaml (v3.0.4)
Loading from cache
- Installing sebastian/version (1.0.6)
Loading from cache
- Installing sebastian/global-state (1.1.1)
Downloading: 100%
- Installing sebastian/recursion-context (1.0.2)
Downloading: 100%
- Installing sebastian/exporter (1.2.1)
Downloading: 100%
- Installing sebastian/environment (1.3.5)
Loading from cache
- Installing sebastian/diff (1.4.1)
Downloading: 100%
- Installing sebastian/comparator (1.2.0)
Downloading: 100%
- Installing phpunit/php-text-template (1.2.1)
Downloading: 100%
- Installing doctrine/instantiator (1.0.5)
Downloading: 100%
- Installing phpunit/phpunit-mock-objects (2.3.8)
Loading from cache
- Installing phpunit/php-timer (1.0.7)
Downloading: 100%
- Installing phpunit/php-file-iterator (1.4.1)
Downloading: 100%
- Installing phpunit/php-code-coverage (2.2.4)
Loading from cache
- Installing phpspec/prophecy (v1.6.0)
Loading from cache
- Installing phpunit/phpunit (4.8.24)
Loading from cache
phpdocumentor/reflection-docblock suggests installing dflydev/markdown (~1.0)
phpdocumentor/reflection-docblock suggests installing erusev/parsedown (~1.0)
sebastian/global-state suggests installing ext-uopz (*)
phpunit/phpunit suggests installing phpunit/php-invoker (~1.1)
Generating autoload files
- I don't understand your point, what you mean with "so many hoops"? I don't share your conception about the README file, since I think it isn't only for beginners but a documentation piece that must cover the main concepts of the project which is described within it.
|
I don't think I will accept this PR at this time. If it does not work for me out of box, it will not work for most beginners who are just trying to play with the SDK. The Readme is intended for new users to jump-start on the SDK with minimal efforts and minimum configuration work. Thanks a lot for your contribution. |
|
Please @yaqiyang, tell me in which way it does not work in order to make the required changes. |
|
Sorry, re-reading your previous comment I've found that you want to keep the example about how to create manually the requirement at |
|
I've reverted the removal and re-worded the section in order to show both methods. Please let me know what do you think. |
| @@ -64,7 +64,24 @@ To get the source code from GitHub, type | |||
| 1. Make sure you have installed **[Compoeser](https://getcomposer.org/doc/00-intro.md#introduction)** in your system. | |||
There was a problem hiding this comment.
Can you move this to the "Add requirement and install with one command" section?
There was a problem hiding this comment.
I think this operation applies for both cases. This library shouldn't be responsible to provide instructions on how install 3rd party tools. Take into account that the installation process can change in any moment, so it is better to point the users to the original source. Indeed, the recommended install process for Composer has been updated a short time ago.
There was a problem hiding this comment.
Because the install process for Composer can change, and I don't want new users to be distracted by that, I want them just "Download composer.phar in your project root" and it will work. No need to configure it locally or globally. It will always work. Your instruction "php composer install " will only work when configured globally.
There was a problem hiding this comment.
Ok, taking your concern into account I'll update all the references based on composer.phar being available at project's root.
There was a problem hiding this comment.
Note that the new install process states the following commands in order to ensure package consistency, security and system settings:
php -r "readfile('https://getcomposer.org/installer');" > composer-setup.php
php -r "if (hash('SHA384', file_get_contents('composer-setup.php')) === '7228c001f88bee97506740ef0888240bd8a760b046ee16db8f4095c0d8d525f2367663f22a46b48d072c816e7fe19959') { echo 'Installer verified'; } else { echo 'Installer corrupt'; unlink('composer-setup.php'); } echo PHP_EOL;"
php composer-setup.php
php -r "unlink('composer-setup.php');"
With your request, all of these checks and features will be lost.
There was a problem hiding this comment.
that is checking the integrity of the installer. if the users downloads composer.phar, they don't have this risk at all
There was a problem hiding this comment.
These lines obviously are checking the integrity of the installer, but the installer itself has a lot of other checks. You can take a look at the composer repo.
Whatever, downloading a file directly from a remote server without any check of integrity (like you are proposing) is very vulnerable for any kind of attack, like MITM by instance.
You can dive into this topic at this issue and take it just as an example.
| Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR |
|
@phansys , let me know if it is ok to close this PR. It will not be accepted as it. Yaqi |
|
Sure @yaqiyang, you're welcome. |
