Improve check php #131

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
5 participants
@KathrynG
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? n/a
Fixed tickets symfony/symfony#10881
License MIT
Doc PR -
Resources/skeleton/app/check.php
+ }
+}
+
+echo PHP_EOL.'Note the command console could use a diffrent php.ini file'.PHP_EOL;

This comment has been minimized.

@javiereguiluz

javiereguiluz May 16, 2014

Member

typo: "diffrent" --> "different"

@javiereguiluz

javiereguiluz May 16, 2014

Member

typo: "diffrent" --> "different"

Resources/skeleton/app/check.php
$checkPassed = true;
+$message = array();

This comment has been minimized.

@javiereguiluz

javiereguiluz May 16, 2014

Member

Very minor suggestion: I would call this variable $messages instead of $message beacuse it's meant to store several error and warning messages.

@javiereguiluz

javiereguiluz May 16, 2014

Member

Very minor suggestion: I would call this variable $messages instead of $message beacuse it's meant to store several error and warning messages.

Resources/skeleton/app/check.php
- echo_requirement($req);
+ if ($helpText = getHelpText($req)) {
+ echo 'E';
+ $message['E'][] = $helpText;

This comment has been minimized.

@javiereguiluz

javiereguiluz May 16, 2014

Member

Another minor suggestion: I would choose error for the key of the array instead of just E for code readability reasons. The same goes for using warning instead of just W.

@javiereguiluz

javiereguiluz May 16, 2014

Member

Another minor suggestion: I would choose error for the key of the array instead of just E for code readability reasons. The same goes for using warning instead of just W.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz May 16, 2014

Member

Please, don't merge this PR yet because we're thinking about some small but important tweaks to the output of the check script. It will be probably ready on Monday.

Member

javiereguiluz commented May 16, 2014

Please, don't merge this PR yet because we're thinking about some small but important tweaks to the output of the check script. It will be probably ready on Monday.

Resources/skeleton/app/check.php
}
+
+ return $requirement->getTestMessage().PHP_EOL.' -> '.$requirement->getHelpText();

This comment has been minimized.

@pyrech

pyrech May 23, 2014

Contributor

What about using the trick from symfony/symfony-standard#653 (requires to pass the $lineSize variable to this function)? It would make the help text more readable IMO :

return $requirement->getTestMessage().PHP_EOL.'    -> '.wordwrap($requirement->getHelpText(), $lineSize - 7, PHP_EOL.'       ');
@pyrech

pyrech May 23, 2014

Contributor

What about using the trick from symfony/symfony-standard#653 (requires to pass the $lineSize variable to this function)? It would make the help text more readable IMO :

return $requirement->getTestMessage().PHP_EOL.'    -> '.wordwrap($requirement->getHelpText(), $lineSize - 7, PHP_EOL.'       ');
@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz May 24, 2014

Member

Applying the suggestion made by @pyrech and adding the following changes, we could improve the output of this script:

diff --git a/Resources/skeleton/app/check.php b/Resources/skeleton/app/check.php
index 5d05fc1..8eb7c5d 100644
--- a/Resources/skeleton/app/check.php
+++ b/Resources/skeleton/app/check.php
@@ -4,27 +4,27 @@ require_once dirname(__FILE__).'/SymfonyRequirements.php';

 $symfonyRequirements = new SymfonyRequirements();

-$iniPath = $symfonyRequirements->getPhpIniConfigPath();
-
-$okMessage = '[OK] Your system is ready to execute Symfony2 projects!';
-$errorMessage = '[ERROR] Your system is not ready to execute Symfony2 projects!';
+$okMessage = '[OK] Your system is ready to execute Symfony2 projects';
+$errorMessage = '[ERROR] Your system is not ready to execute Symfony2 projects';

-$lineSize = strlen($errorMessage);
+$lineSize = strlen($errorMessage) + 2;

+echo PHP_EOL;
 echo 'Symfony2 Requirements Checker'.PHP_EOL;
-echo str_repeat('=', $lineSize);
+echo '~~~~~~~~~~~~~~~~~~~~~~~~~~~~~';

-echo_title('Looking for the INI configuration file used by PHP');
+$iniPath = $symfonyRequirements->getPhpIniConfigPath();

+echo_title('Looking for the INI configuration file used by PHP:');
 echo $iniPath ? $iniPath : 'WARNING: No configuration file (php.ini) used by PHP!';

-echo_title('Checking mandatory requirements:');
+echo_title('Checking Symfony requirements:');

 $checkPassed = true;
 $messages = array();
 foreach ($symfonyRequirements->getRequirements() as $req) {
     /** @var $req Requirement */
-    if ($helpText = getErrorMessage($req)) {
+    if ($helpText = getErrorMessage($req, $lineSize)) {
         echo 'E';
         $messages['error'][] = $helpText;
     } else {
@@ -36,10 +36,8 @@ foreach ($symfonyRequirements->getRequirements() as $req) {
     }
 }

-echo_title('Checking optional recommendations:');
-
 foreach ($symfonyRequirements->getRecommendations() as $req) {
-    if ($helpText = getErrorMessage($req)) {
+    if ($helpText = getErrorMessage($req, $lineSize)) {
         echo 'W';
         $messages['warning'][] = $helpText;
     } else {
@@ -55,48 +53,56 @@ if (!empty($messages['error'])) {
     echo_result($errorMessage, $lineSize);

     echo PHP_EOL.'Fix the following mandatory requirements'.PHP_EOL;
-    echo str_repeat('-', $lineSize).PHP_EOL;
+    echo str_repeat('~', $lineSize).PHP_EOL;

     foreach ($messages['error'] as $helpText) {
-        echo '  * '.$helpText.PHP_EOL;
+        echo ' * '.$helpText.PHP_EOL;
     }
 }

 if (!empty($messages['warning'])) {
-    echo PHP_EOL.'(Optional) Fix the following recommendations'.PHP_EOL;
-    echo str_repeat('-', $lineSize).PHP_EOL;
+    echo PHP_EOL.'Optional recommendations to improve your setup'.PHP_EOL;
+    echo str_repeat('~', $lineSize).PHP_EOL;

     foreach ($messages['warning'] as $helpText) {
-        echo '  * '.$helpText.PHP_EOL;
+        echo ' * '.$helpText.PHP_EOL;
     }
 }

-echo PHP_EOL.'Note  the command console could use a different php.ini file'.PHP_EOL;
+echo PHP_EOL;
+echo 'Note  the command console could use a different php.ini file'.PHP_EOL;
 echo '~~~~  than the one used with your web server. To be on the'.PHP_EOL;
 echo '      safe side, please check the requirements from your web'.PHP_EOL;
 echo '      server using the web/config.php script.'.PHP_EOL;
+echo PHP_EOL;

 exit($checkPassed ? 0 : 1);

-function getErrorMessage(Requirement $requirement)
+function getErrorMessage(Requirement $requirement, $lineSize)
 {
     if ($requirement->isFulfilled()) {
         return;
     }

-    return $requirement->getTestMessage().PHP_EOL.'    -> '.$requirement->getHelpText();
+    $errorMessage  = wordwrap($requirement->getTestMessage(), $lineSize - 3, PHP_EOL.'   ').PHP_EOL;
+    $errorMessage .= '   > '.wordwrap($requirement->getHelpText(), $lineSize - 5, PHP_EOL.'   > ').PHP_EOL;
+
+    return $errorMessage;
 }

 function echo_title($title)
 {
-    echo PHP_EOL.PHP_EOL.'> '.$title.PHP_EOL;
+    echo PHP_EOL.PHP_EOL;
+    echo '> '.$title.PHP_EOL;
+    echo '  ';
 }

 function echo_result($message, $lineSize)
 {
     echo PHP_EOL.PHP_EOL;
-    echo str_repeat('=', $lineSize).PHP_EOL;
-    echo $message.PHP_EOL;
-    echo str_repeat('=', $lineSize).PHP_EOL;
-    sleep(1);
+    echo '+'.str_repeat('-', $lineSize).'+'.PHP_EOL;
+    echo '|'.str_repeat(' ', $lineSize).'|'.PHP_EOL;
+    echo '| '.$message.str_repeat(' ', $lineSize - strlen($message) - 2).' |'.PHP_EOL;
+    echo '|'.str_repeat(' ', $lineSize).'|'.PHP_EOL;
+    echo '+'.str_repeat('-', $lineSize).'+'.PHP_EOL;
 }

Here are some comparison screenshots:

Best case - No errors or warnings

check_best_case

Errors only

check_requirements

Warnings only

check_recommendations

Worst case - Lots of errors and warnings

check_worst_case

Member

javiereguiluz commented May 24, 2014

Applying the suggestion made by @pyrech and adding the following changes, we could improve the output of this script:

diff --git a/Resources/skeleton/app/check.php b/Resources/skeleton/app/check.php
index 5d05fc1..8eb7c5d 100644
--- a/Resources/skeleton/app/check.php
+++ b/Resources/skeleton/app/check.php
@@ -4,27 +4,27 @@ require_once dirname(__FILE__).'/SymfonyRequirements.php';

 $symfonyRequirements = new SymfonyRequirements();

-$iniPath = $symfonyRequirements->getPhpIniConfigPath();
-
-$okMessage = '[OK] Your system is ready to execute Symfony2 projects!';
-$errorMessage = '[ERROR] Your system is not ready to execute Symfony2 projects!';
+$okMessage = '[OK] Your system is ready to execute Symfony2 projects';
+$errorMessage = '[ERROR] Your system is not ready to execute Symfony2 projects';

-$lineSize = strlen($errorMessage);
+$lineSize = strlen($errorMessage) + 2;

+echo PHP_EOL;
 echo 'Symfony2 Requirements Checker'.PHP_EOL;
-echo str_repeat('=', $lineSize);
+echo '~~~~~~~~~~~~~~~~~~~~~~~~~~~~~';

-echo_title('Looking for the INI configuration file used by PHP');
+$iniPath = $symfonyRequirements->getPhpIniConfigPath();

+echo_title('Looking for the INI configuration file used by PHP:');
 echo $iniPath ? $iniPath : 'WARNING: No configuration file (php.ini) used by PHP!';

-echo_title('Checking mandatory requirements:');
+echo_title('Checking Symfony requirements:');

 $checkPassed = true;
 $messages = array();
 foreach ($symfonyRequirements->getRequirements() as $req) {
     /** @var $req Requirement */
-    if ($helpText = getErrorMessage($req)) {
+    if ($helpText = getErrorMessage($req, $lineSize)) {
         echo 'E';
         $messages['error'][] = $helpText;
     } else {
@@ -36,10 +36,8 @@ foreach ($symfonyRequirements->getRequirements() as $req) {
     }
 }

-echo_title('Checking optional recommendations:');
-
 foreach ($symfonyRequirements->getRecommendations() as $req) {
-    if ($helpText = getErrorMessage($req)) {
+    if ($helpText = getErrorMessage($req, $lineSize)) {
         echo 'W';
         $messages['warning'][] = $helpText;
     } else {
@@ -55,48 +53,56 @@ if (!empty($messages['error'])) {
     echo_result($errorMessage, $lineSize);

     echo PHP_EOL.'Fix the following mandatory requirements'.PHP_EOL;
-    echo str_repeat('-', $lineSize).PHP_EOL;
+    echo str_repeat('~', $lineSize).PHP_EOL;

     foreach ($messages['error'] as $helpText) {
-        echo '  * '.$helpText.PHP_EOL;
+        echo ' * '.$helpText.PHP_EOL;
     }
 }

 if (!empty($messages['warning'])) {
-    echo PHP_EOL.'(Optional) Fix the following recommendations'.PHP_EOL;
-    echo str_repeat('-', $lineSize).PHP_EOL;
+    echo PHP_EOL.'Optional recommendations to improve your setup'.PHP_EOL;
+    echo str_repeat('~', $lineSize).PHP_EOL;

     foreach ($messages['warning'] as $helpText) {
-        echo '  * '.$helpText.PHP_EOL;
+        echo ' * '.$helpText.PHP_EOL;
     }
 }

-echo PHP_EOL.'Note  the command console could use a different php.ini file'.PHP_EOL;
+echo PHP_EOL;
+echo 'Note  the command console could use a different php.ini file'.PHP_EOL;
 echo '~~~~  than the one used with your web server. To be on the'.PHP_EOL;
 echo '      safe side, please check the requirements from your web'.PHP_EOL;
 echo '      server using the web/config.php script.'.PHP_EOL;
+echo PHP_EOL;

 exit($checkPassed ? 0 : 1);

-function getErrorMessage(Requirement $requirement)
+function getErrorMessage(Requirement $requirement, $lineSize)
 {
     if ($requirement->isFulfilled()) {
         return;
     }

-    return $requirement->getTestMessage().PHP_EOL.'    -> '.$requirement->getHelpText();
+    $errorMessage  = wordwrap($requirement->getTestMessage(), $lineSize - 3, PHP_EOL.'   ').PHP_EOL;
+    $errorMessage .= '   > '.wordwrap($requirement->getHelpText(), $lineSize - 5, PHP_EOL.'   > ').PHP_EOL;
+
+    return $errorMessage;
 }

 function echo_title($title)
 {
-    echo PHP_EOL.PHP_EOL.'> '.$title.PHP_EOL;
+    echo PHP_EOL.PHP_EOL;
+    echo '> '.$title.PHP_EOL;
+    echo '  ';
 }

 function echo_result($message, $lineSize)
 {
     echo PHP_EOL.PHP_EOL;
-    echo str_repeat('=', $lineSize).PHP_EOL;
-    echo $message.PHP_EOL;
-    echo str_repeat('=', $lineSize).PHP_EOL;
-    sleep(1);
+    echo '+'.str_repeat('-', $lineSize).'+'.PHP_EOL;
+    echo '|'.str_repeat(' ', $lineSize).'|'.PHP_EOL;
+    echo '| '.$message.str_repeat(' ', $lineSize - strlen($message) - 2).' |'.PHP_EOL;
+    echo '|'.str_repeat(' ', $lineSize).'|'.PHP_EOL;
+    echo '+'.str_repeat('-', $lineSize).'+'.PHP_EOL;
 }

Here are some comparison screenshots:

Best case - No errors or warnings

check_best_case

Errors only

check_requirements

Warnings only

check_recommendations

Worst case - Lots of errors and warnings

check_worst_case

@pyrech

This comment has been minimized.

Show comment
Hide comment
@pyrech

pyrech May 26, 2014

Contributor

Good job! ;)

Contributor

pyrech commented May 26, 2014

Good job! ;)

@KathrynG

This comment has been minimized.

Show comment
Hide comment
@KathrynG

KathrynG May 27, 2014

Looking good! :)

Looking good! :)

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot May 27, 2014

Member

Just a small suggestion: what about using colors to emphasize the main result: ok or ko? probably green vs red.

Member

fabpot commented May 27, 2014

Just a small suggestion: what about using colors to emphasize the main result: ok or ko? probably green vs red.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz May 27, 2014

Member

@fabpot colors are definitely a very good idea! The problem is that this script must only use raw and simple PHP code to always execute correctly. I don't know if it's easy to add colors by hand and to make them compatible with any kind of PHP installation and Operating System.

Member

javiereguiluz commented May 27, 2014

@fabpot colors are definitely a very good idea! The problem is that this script must only use raw and simple PHP code to always execute correctly. I don't know if it's easy to add colors by hand and to make them compatible with any kind of PHP installation and Operating System.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot May 27, 2014

Member

Just get some piece of code from the console. Using colors is as easy as it can get and detecting if the user supports colors is a bit more involving the work has already been done in the console component.

Member

fabpot commented May 27, 2014

Just get some piece of code from the console. Using colors is as easy as it can get and detecting if the user supports colors is a bit more involving the work has already been done in the console component.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz May 28, 2014

Member

We are going to add the following code to add color support:

function echo_result($message, $lineSize, $type)
{
    // ANSI color codes
    $colorCodes = array(
        "none"  => "\033[0m",
        "ok"    => "\033[32m",
        "error" => "\033[37;41m",
    );

    $lineStart = hasColorSupport() ? $colorCodes[$type] : '';
    $lineEnd   = hasColorSupport() ? $colorCodes['none'].PHP_EOL : PHP_EOL;

    echo PHP_EOL.PHP_EOL;
    echo $lineStart.'+'.str_repeat('-', $lineSize).'+'.$lineEnd;
    echo $lineStart.'|'.str_repeat(' ', $lineSize).'|'.$lineEnd;
    echo $lineStart.'| '.$message.str_repeat(' ', $lineSize - strlen($message) - 2).' |'.$lineEnd;
    echo $lineStart.'|'.str_repeat(' ', $lineSize).'|'.$lineEnd;
    echo $lineStart.'+'.str_repeat('-', $lineSize).'+'.$lineEnd;
}

function hasColorSupport()
{
    if (DIRECTORY_SEPARATOR == '\\') {
        return false !== getenv('ANSICON') || 'ON' === getenv('ConEmuANSI');
    }

    return function_exists('posix_isatty') && @posix_isatty(STDOUT);
}

For consistency, the colors are exactly the same as the <info> and <error> styles from the Symfony Console. Here are the screenshots:

color_ok

color_errors

Member

javiereguiluz commented May 28, 2014

We are going to add the following code to add color support:

function echo_result($message, $lineSize, $type)
{
    // ANSI color codes
    $colorCodes = array(
        "none"  => "\033[0m",
        "ok"    => "\033[32m",
        "error" => "\033[37;41m",
    );

    $lineStart = hasColorSupport() ? $colorCodes[$type] : '';
    $lineEnd   = hasColorSupport() ? $colorCodes['none'].PHP_EOL : PHP_EOL;

    echo PHP_EOL.PHP_EOL;
    echo $lineStart.'+'.str_repeat('-', $lineSize).'+'.$lineEnd;
    echo $lineStart.'|'.str_repeat(' ', $lineSize).'|'.$lineEnd;
    echo $lineStart.'| '.$message.str_repeat(' ', $lineSize - strlen($message) - 2).' |'.$lineEnd;
    echo $lineStart.'|'.str_repeat(' ', $lineSize).'|'.$lineEnd;
    echo $lineStart.'+'.str_repeat('-', $lineSize).'+'.$lineEnd;
}

function hasColorSupport()
{
    if (DIRECTORY_SEPARATOR == '\\') {
        return false !== getenv('ANSICON') || 'ON' === getenv('ConEmuANSI');
    }

    return function_exists('posix_isatty') && @posix_isatty(STDOUT);
}

For consistency, the colors are exactly the same as the <info> and <error> styles from the Symfony Console. Here are the screenshots:

color_ok

color_errors

@KathrynG

This comment has been minimized.

Show comment
Hide comment
@KathrynG

KathrynG Jun 2, 2014

Just commited the changes for the colours in the output text.

KathrynG commented Jun 2, 2014

Just commited the changes for the colours in the output text.

@javiereguiluz javiereguiluz referenced this pull request in symfony/symfony Jun 18, 2014

Closed

Improving the app/check.php script #10881

@csarrazi

This comment has been minimized.

Show comment
Hide comment
@csarrazi

csarrazi Jun 18, 2014

How about removing the dash, pipe and plus signs, when in color mode? Relying on the background should be enough in this case.

How about removing the dash, pipe and plus signs, when in color mode? Relying on the background should be enough in this case.

@jubianchi jubianchi referenced this pull request in KathrynG/SensioDistributionBundle Jul 5, 2014

Closed

Add more styling #1

@pyrech pyrech referenced this pull request in symfony/symfony-standard Jul 18, 2014

Closed

More readable results of the check requirement in console #683

@fabpot fabpot referenced this pull request in symfony/symfony-standard Jul 19, 2014

Closed

Improved app/check.php output #653

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jul 19, 2014

Member

Thanks @KathrynG.

Member

fabpot commented Jul 19, 2014

Thanks @KathrynG.

@fabpot fabpot closed this Jul 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment