Skip to content
This repository was archived by the owner on Nov 14, 2019. It is now read-only.

Issue #42 A better way to manage the version on install #90

Closed
wants to merge 8 commits into from
Closed

Issue #42 A better way to manage the version on install #90

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f75f252
Issue #42 A better way to manage the version on install
Pierstoval Dec 22, 2014
33e4966
Fixes for PR #90 (thx @xabbuh)
Pierstoval Dec 23, 2014
d8ac816
Rewind back `getExecutedCommand()` after f75f252
Pierstoval Dec 23, 2014
a53a2e8
Fix regex mistake, added some details in comments
Pierstoval Dec 23, 2014
3bdaaeb
Use of "json()" method on response,to avoid Guzzle's.
Pierstoval Dec 26, 2014
68b8d68
Fix @stof note about non-capturing group in regex
Pierstoval Dec 27, 2014
317d0b9
Added an error message in case of the selected branch is incorrect
Pierstoval Dec 27, 2014
8a892ab
Removed 'latest' keyword as it is the default value
Pierstoval Dec 28, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions src/Symfony/Installer/NewCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Distill\Exception\IO\Input\FileEmptyException;
use Distill\Exception\IO\Output\TargetDirectoryNotWritableException;
use Distill\Strategy\MinimumSize;
use GuzzleHttp;
use GuzzleHttp\Client;
use GuzzleHttp\Exception\ClientException;
use GuzzleHttp\Message\Response;
Expand Down Expand Up @@ -122,9 +123,24 @@ private function checkSymfonyVersionIsInstallable()
return $this;
}

// validate semver syntax
if (!preg_match('/^2\.\d\.\d{1,2}$/', $this->version)) {
throw new \RuntimeException('The Symfony version should be 2.N.M, where N = 0..9 and M = 0..99');
// validate server syntax
if (!preg_match('/^(2\.\d\.\d{1,2}|2\.\d)$/', $this->version)) {
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 use parentheses to distinguish between the two forms of versions:

/^(2\.\d\.\d{1,2})|(2\.\d)$/

throw new \RuntimeException('The Symfony version should be 2.N or 2.N.M, where N = 0..9 and M = 0..99');
}

if (preg_match('/^(2\.\d\.\d{1,2})|(2\.\d)$/', $this->version)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you do the check again here? If the regex didn't match, an exception would have been thrown and this code wouldn't be reached.

Copy link
Member

Choose a reason for hiding this comment

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

But you could check for ^2\.d$ to only execute the following lines if the user didn't specify a patch version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact I meant to check 2\.[0-9]+ , I saw your note about changing the regex but now I see that these lines look alike very VERY much in the previous diff, and I realize that I should have never changed this regex in fact.
I'm gonna change it, but as a reminder, this line should only check for minor versions, so I'll change back the regex to 2\.\d+

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I just had a look at 33e4966 and it seems that you just modified the wrong regex. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, indeed.
Moreover, would it be good to rewrite these regexes for higher minor/patch versions compatibility ?
I mean, on the line 126, changing /^(2\.\d\.\d{1,2}|2\.\d)$/ for /^2\.\d+(\.\d+)?$/ ? It's more readable to me but I don't know if it's a better practice for other developers :/

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 /^2\.\d(\.\d{1,2})?$/ is fine (just keep {1,2} here, iirc it was added on purpose).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok fine.

When Symfony3 will be released for first stabilization process, we may change the 2\. part into [23]\. in every regex , don't we ?

// Check if we have a minor version

$client = new Client();
$json = $client->get('http://symfony.com/versions.json')->getBody()->getContents();

if ($json) {
$versionsList = GuzzleHttp\json_decode($json, true);
Copy link
Member

Choose a reason for hiding this comment

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

why not using json_decode directly ?

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 that Guzzle's json_decode has a better error management than the original one

Copy link
Member

Choose a reason for hiding this comment

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

but this function is gone in 5.x, which would make it harder to bump our requirement to the current version (which is something I would like to do)

and instead of doing $json = $response->getBody()->getContents(); json_decode($json);, you could simply use $response->json(), which gives you the decoded json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do you recommend to entirely use json_decode instead of using Guzzle ?

Wasn't aware of $response->json() , thanks for the nice tip

if (isset($versionsList[$this->version])) {
// Get the latest installable of the minor version the user asked
$this->version = $versionsList[$this->version];
}
}
}

// 2.0, 2.1, 2.2 and 2.4 cannot be installed because they are unmaintained
Expand Down Expand Up @@ -241,7 +257,7 @@ private function download()
));
} else {
throw new \RuntimeException(sprintf(
"The selected version (%s) couldn\'t be downloaded because of the following error:\n%s",
"The selected version (%s) couldn't be downloaded because of the following error:\n%s",
$this->version,
$e->getMessage()
));
Expand Down Expand Up @@ -553,6 +569,7 @@ private function generateComposerProjectName()
/**
* Returns the executed command.
*
* @param string $version A version to add to the command as example, if needed.
Copy link
Member

Choose a reason for hiding this comment

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

You would also have to remove this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crap, not enough coffee for me this morning, thanks for your note

* @return string
*/
private function getExecutedCommand()
Expand Down