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

Pierstoval · 2014-12-22T20:37:25Z

No description provided.

xabbuh · 2014-12-22T20:50:47Z

src/Symfony/Installer/NewCommand.php

-        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)) {


I would use parentheses to distinguish between the two forms of versions:

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

stof · 2014-12-23T08:50:44Z

src/Symfony/Installer/NewCommand.php

     * @return string
     */
-    private function getExecutedCommand()
+    private function getExecutedCommand($version = '')


this argument does not seem to be used

Yup, was used in the first commit when throwing an exception which proposed to the user to install the latest version instead of the one he was trying to install.
I'm gonna remove it, as this error has been deleted in the last fix commit

Fixes for #90, the exception when checking for minor version is no more thrown so the `$version` behavior has to come back to its original mode.

xabbuh · 2014-12-23T09:25:21Z

src/Symfony/Installer/NewCommand.php

+            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)) {


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.

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

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+

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

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 :/

I think /^2\.\d(\.\d{1,2})?$/ is fine (just keep {1,2} here, iirc it was added on purpose).

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 ?

Pierstoval · 2014-12-26T19:57:05Z

Voilà ! Relied on the json() method from the ResponseInterface, in order to facilitate the modification of the Guzzle's Response.

I think we're good for this issue, aren't we ?

stof · 2014-12-27T00:21:11Z

src/Symfony/Installer/NewCommand.php

-        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})?$/', $this->version)) {


please use a non-capturing group

Pierstoval · 2014-12-27T09:30:07Z

Fixed @stof's note about the regexp :)

xabbuh · 2014-12-27T09:33:53Z

We should probably provide a sensible error message if the installer wasn't able to find the latest patch version.

Pierstoval · 2014-12-27T17:57:04Z

@xabbuh Here it is :) I added the message only when $versionsList is not empty, because if it is, it means that Guzzle couldn't connect to symfony.com, so the user won't be able to download Symfony anyway.

xabbuh · 2014-12-28T15:35:23Z

src/Symfony/Installer/NewCommand.php

+                throw new \RuntimeException(sprintf(
+                    "The selected branch (%s) does not exist, or is not maintained.\n".
+                    "To solve this issue, install Symfony with the latest stable release:\n\n".
+                    '%s %s %s latest',


latest is the default value and you can omit it here.

Pierstoval · 2015-01-02T14:18:30Z

Hi @xabbuh , is this PR ok for merging ? 😄

xabbuh · 2015-01-02T14:20:58Z

@Pierstoval I'm not the one to decide, but to me it looks ready. :)

Pierstoval · 2015-01-02T14:22:24Z

Fine, thanks ! We'll just wait for it to be merged, I think it's an awesome feature !
I just installed a new SF2.6 app some days ago and was surprised that I couldn't do "new app/ 2.6" , when I remembered that it was only available in my local machine 😆

javiereguiluz · 2015-01-02T14:38:12Z

Thanks @Pierstoval for working on this feature. The Symfony Installer is now almost complete!

introduced by symfony#90

… introduced by #90 (javiereguiluz) This PR was merged into the 1.0-dev branch. Discussion ---------- Updated the Installer instructions to keep with the changes introduced by #90 Commits ------- 40064a5 Updated the Installer instructions to keep with the changes introduced by #90

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

f75f252

xabbuh reviewed Dec 22, 2014
View reviewed changes

Fixes for PR #90 (thx @xabbuh)

33e4966

stof reviewed Dec 23, 2014
View reviewed changes

Rewind back getExecutedCommand() after f75f252

d8ac816

Fixes for #90, the exception when checking for minor version is no more thrown so the `$version` behavior has to come back to its original mode.

xabbuh reviewed Dec 23, 2014
View reviewed changes

Fix regex mistake, added some details in comments

a53a2e8

javiereguiluz mentioned this pull request Dec 24, 2014

Feature issue #82 Add a command to list versions #84

Closed

Use of "json()" method on response,to avoid Guzzle's.

3bdaaeb

stof reviewed Dec 27, 2014
View reviewed changes

Fix @stof note about non-capturing group in regex

68b8d68

Added an error message in case of the selected branch is incorrect

317d0b9

xabbuh reviewed Dec 28, 2014
View reviewed changes

Removed 'latest' keyword as it is the default value

8a892ab

javiereguiluz closed this in 1b1899e Jan 2, 2015

javiereguiluz added a commit to javiereguiluz/symfony-installer that referenced this pull request Jan 2, 2015

Updated the Installer instructions to keep with the changes

40064a5

introduced by symfony#90

Pierstoval deleted the issue42 branch February 28, 2015 10:20

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

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

Uh oh!

Conversation

Pierstoval commented Dec 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pierstoval commented Dec 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pierstoval commented Dec 27, 2014

Uh oh!

xabbuh commented Dec 27, 2014

Uh oh!

Pierstoval commented Dec 27, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pierstoval commented Jan 2, 2015

Uh oh!

xabbuh commented Jan 2, 2015

Uh oh!

Pierstoval commented Jan 2, 2015

Uh oh!

javiereguiluz commented Jan 2, 2015

Uh oh!

Uh oh!