Check laravel version before symlink storage #1246

lucasmezencio · 2017-05-28T02:14:35Z

Since Laravel 5.2 doesn't have artisan storage:link command, we can't call on versions below 5.3.

Q	A
Bug fix?	No
New feature?	Yes
BC breaks?	No
Deprecations?	No
Fixed tickets	N/A

Do not forget to add notes about your changes to CHANGELOG.md

Since Laravel 5.2 does not have `artisan storage:link` command, we can't call it.

antonmedv · 2017-05-28T06:29:17Z

recipe/laravel.php

@@ -105,7 +105,20 @@

 desc('Execute artisan storage:link');
 task('artisan:storage:link', function () {
-    run('{{bin/php}} {{release_path}}/artisan storage:link');
+    if (get('laravel_version') > 5.2) {


Use version-compare http://php.net/manual/ru/function.version-compare.php

antonmedv · 2017-05-28T06:30:22Z

recipe/laravel.php

+});
+
+desc('Get Laravel version');
+task('artisan:version', function () {


Better define a as callback:

set('laravel_version', function () { ... });

lucasmezencio · 2017-05-28T19:21:13Z

@antonmedv done, man. :)

antonmedv · 2017-05-30T12:25:24Z

CHANGELOG.md

@@ -5,6 +5,8 @@

 ### Fixed
 - Fixed upload / download with optional rsync ssh options [#1227]
+- Fixed storage link error when deploying Laravel < 5.3. 
+- Helps [#1153]


What is helps?) Add link also in bottom of document.

@antonmedv this PR helps that issue. Sure, I will do that.

barryvdh · 2017-05-30T12:28:50Z

recipe/laravel.php

+
+    $version = $matches[1][0] ?? 5.4;
+
+    return (float) $version;


version_compare expects a string, no need to cast to float. Can only cause rounding errors.

@barryvdh maybe I can change the regex to return the right version, but I did this because 5.2.13 > 5.2, so we can get a false positive. What do you think?

See the docs on http://php.net/manual/en/function.version-compare.php
That is specifically created to handle those situations.

See https://3v4l.org/a0jb5 for examples

barryvdh · 2017-05-30T12:32:29Z

recipe/laravel.php

+set('laravel_version', function () {
+    $result = run('{{bin/php}} {{release_path}}/artisan --version');
+
+    preg_match_all('/version\ (.+?)$/', $result, $matches);


Where does 'version' come from? My result is Laravel Framework 5.4.12

@barryvdh hmmm. Make sense! In Laravel 5.2, --version param come with 'version' string. I will change it. Thanks.

Laravel 4: Laravel Framework version 4.2.22
Laravel 5.1: Laravel Framework version 5.1.35 (LTS)
Laravel 5.2: Laravel Framework version 5.2.45
Laravel 5.3: Laravel Framework version 5.3.30
Laravel 5.4: Laravel Framework 5.4.22
Lumen: Laravel Framework version Lumen (5.1.7) (Laravel Components 5.1.*)

Something like this? ([0-9.]+)
http://regexr.com/3g2ep

antonmedv · 2017-05-30T12:51:40Z

And please rebase.

barryvdh · 2017-05-30T12:55:15Z

recipe/laravel.php

@@ -105,7 +115,9 @@

 desc('Execute artisan storage:link');
 task('artisan:storage:link', function () {
-    run('{{bin/php}} {{release_path}}/artisan storage:link');
+    if (version_compare(get('laravel_version'), 5.2, '>')) {


And as you say in the docs, you want to check if the version is >= 5.3 or NOT < 5.3

@barryvdh sure sure! You are right! I will do the changes. Thanks, man!

lucasmezencio · 2017-05-30T20:42:45Z

@antonmedv @barryvdh can you check please? :)

antonmedv · 2017-05-31T06:03:47Z

@lucasmezencio nice work!

barryvdh · 2017-05-31T10:07:10Z

recipe/laravel.php

+set('laravel_version', function () {
+    $result = run('{{bin/php}} {{release_path}}/artisan --version');
+
+    preg_match_all('/([0-9\.])$/', $result, $matches);


Wrong, should be '/([0-9.]+)/'. You want to match ALL numbers/dots, not just 1. And it doesn't have to match the end of string.

lucasmezencio added 2 commits May 27, 2017 23:09

Add Laravel version task

6229d68

Check Laravel version before run artisan storage:link

c0f927f

Since Laravel 5.2 does not have `artisan storage:link` command, we can't call it.

antonmedv requested changes May 28, 2017

View reviewed changes

antonmedv added the feature label May 28, 2017

lucasmezencio added 2 commits May 28, 2017 16:18

Change from task to configuration option

54b589f

Compare versions using PHP's built-in function version_compare()

7abcbbe

lucasmezencio mentioned this pull request May 30, 2017

Versioning framework recipes #1153

Closed

Update changelog

ecc85db

antonmedv reviewed May 30, 2017

View reviewed changes

antonmedv approved these changes May 30, 2017

View reviewed changes

barryvdh reviewed May 30, 2017

View reviewed changes

barryvdh and others added 8 commits May 30, 2017 17:32

Disable maintenance after failure

5e3feb9

Update CHANGELOG.md

724fa20

Update changelog

03bbf7e

Fix version regex and change version check

69c8f7c

Update changelog

0bd8d4a

Update changelog

704fc69

Update changelog

910e2e2

Merge branch 'master' into check-laravel-version-before-symlink-storage

9279e2c

antonmedv merged commit 120dae4 into deployphp:master May 31, 2017

barryvdh mentioned this pull request May 31, 2017

Fix a parsing of laravel version in output "php artisan --version" #1252

Merged

barryvdh reviewed May 31, 2017

View reviewed changes

lucasmezencio deleted the check-laravel-version-before-symlink-storage branch June 2, 2017 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check laravel version before symlink storage #1246

Check laravel version before symlink storage #1246

lucasmezencio commented May 28, 2017

antonmedv May 28, 2017

antonmedv May 28, 2017

lucasmezencio commented May 28, 2017

antonmedv May 30, 2017

lucasmezencio May 30, 2017

barryvdh May 30, 2017

lucasmezencio May 30, 2017

barryvdh May 30, 2017

barryvdh May 30, 2017

barryvdh May 30, 2017

lucasmezencio May 30, 2017

barryvdh May 30, 2017 •

edited

Loading

antonmedv commented May 30, 2017

barryvdh May 30, 2017

lucasmezencio May 30, 2017

lucasmezencio commented May 30, 2017

antonmedv commented May 31, 2017

barryvdh May 31, 2017

Check laravel version before symlink storage #1246

Check laravel version before symlink storage #1246

Conversation

lucasmezencio commented May 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmezencio commented May 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barryvdh May 30, 2017 • edited Loading

Choose a reason for hiding this comment

antonmedv commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmezencio commented May 30, 2017

antonmedv commented May 31, 2017

Choose a reason for hiding this comment

barryvdh May 30, 2017 •

edited

Loading