Updated all CLI-valid cases of exit() to return a valid code #2190

danhunsaker · 2013-01-25T00:16:18Z

Specific codes are as follows, but can easily be changed if a different order/breakdown makes more sense:

0: Success; everything executed as planned
1: Configuration Error; something is wrong with/in the configuration file(s)
2: Class Not Found; what it says
3: Driver Method Unsupported; the method you're trying to use on a Driver doesn't exist
4: File Not Found; 404 error
5: Database Error; something is broken in the database somewhere
6: Invalid Input; the user attempted to submit a request with invlaid characters in 1+ key names
7 through 26 are reserved for future use
27: Generic Error; generated by show_error() when the status code is >= 100
28 through 127 are errors generated by user applications, normally by using show_error() with a status code below 100
128 through 254 should not be used by applications, as they are reserved by system-level functions
255: PHP Fatal Error; automatically generated by PHP for fatal errors, and therefore not allowed for our use

Status codes below 100 are shifted up by 28 to place them in the user error range. It may make more sense to have these codes left alone and instead shift the CI errors into the 101 through 127 space, but that's not what I opted for here.

It would probably also be a good idea to replace the hard-coded numbers with constants or some such, but I was in a bit of a hurry when I made these changes, so I didn't look around for the best place to do this. With proper guidance, I could easily amend this commit with another that uses such constant values.

Signed-off-by: Daniel Hunsaker danhunsaker@gmail.com

Specific codes are as follows, but can easily be changed if a different order/breakdown makes more sense: - 0: Success; everything executed as planned - 1: Configuration Error; something is wrong with/in the configuration file(s) - 2: Class Not Found; what it says - 3: Driver Method Unsupported; the method you're trying to use on a Driver doesn't exist - 4: File Not Found; 404 error - 5: Database Error; something is broken in the database somewhere - 6: Invalid Input; the user attempted to submit a request with invlaid characters in 1+ key names 7 through 26 are reserved for future use - 27: Generic Error; generated by show_error() when the status code is >= 100 28 through 127 are errors generated by user applications, normally by using show_error() with a status code below 100 128 through 254 should not be used by applications, as they are reserved by system-level functions - 255: PHP Fatal Error; automatically generated by PHP for fatal errors, and therefore not allowed for our use Status codes below 100 are shifted up by 28 to place them in the user error range. It may make more sense to have these codes left alone and instead shift the CI errors into the 101 through 127 space, but that's not what I opted for here. It would probably also be a good idea to replace the hard-coded numbers with constants or some such, but I was in a bit of a hurry when I made these changes, so I didn't look around for the best place to do this. With proper guidance, I could easily amend this commit with another that uses such constant values. Signed-off-by: Daniel Hunsaker <danhunsaker@gmail.com>

AkenRoberts · 2013-01-25T07:55:56Z

I'm curious where those integer values came from? I can't find any official doc referencing them.

danhunsaker · 2013-01-25T07:59:38Z

Which ones are you curious about? I'll try to dig up my sources for them.

AkenRoberts · 2013-01-25T08:03:33Z

Well, any of them. The docs for exit only make reference to 255 being reserved by PHP, and 0 being a successful execution. This leads me to believe that the codes can be user defined. So just seeing if my assumption is true, or if there's more to it than that.

danhunsaker · 2013-01-25T08:24:40Z

It looks as though the information I was working from was specific to a particular application (don't recall which) rather than actually being system-wide. In fact, the C/C++ standard (/usr/include/sysexits.h on systems which have it) seems to be even more restrictive than whatever environment I was working with above - only 50 codes (less, in more recent versions) for user-defined errors! So yes, the numbers could, technically, be anything between 1 and 254, inclusive.

What is more important is that the numbers are not zero; command line apps don't always work properly otherwise. I am more than open to change these to anything else if the numbers I arbitrarily assigned (1-127) are suboptimal. The "status code[s] below 100" bit is less arbitrary, as 100 is the lowest valid HTTP status code, which is what the existing version of show_error() expects to receive. This could still easily be modified, though, to associate specific HTTP codes with specific exit codes. So a lot of options, here.

AkenRoberts · 2013-01-25T08:45:53Z

Gotcha, thank you for the info. I don't develop for use with a CLI, so I can't comment on the use of exit status codes (I did a little research and it appears the most common uses of exit codes are either with CLI or when using exec() which is not usually recommended). I agree with the idea of defining constants that can be customized per user with their own codes, but since this is such a rare use case that I'm not familiar with, I'll let others offer their more experienced feedback.

danhunsaker · 2013-01-26T11:57:03Z

I think I'll work on modifying this to be more configurable, and with better defaults, then push a revised commit. CLI may not be the primary use case, but there's enough support for it already built into CI that it deserves a more complete implementation. I welcome/request any further feedback on the best way to go about this, especially as regards sane defaults.

Also, documentation!

jbrower · 2013-01-26T16:43:26Z

I agree that adding non-zero exit codes when it exits due to some sort of failure is a very good idea. It wouldn't help much for web requests, but is essential for proper use of the CLI. Excited to see this merged in after you enhance it @danhunsaker.

…e/exit-status

Re-allocated exit status codes according to three references, which follow: BSD sysexits.h:http://www.gsp.com/cgi-bin/man.cgi?section=3&topic=sysexits GNU recomendations:http://www.gnu.org/software/libc/manual/html_node/Exit-Status.html Bash scripting:http://tldp.org/LDP/abs/html/exitcodes.html The GNU recommendations stem from and expand upon the standard C/C++ library (stdlibc) definitions, while also suggesting some best-practice conventions which happen to prevent exit status code collisions with bash, and probably other shells. The re-allocated codes are now mapped to constant values, set in *application/config/constants.php*, and used throughout the CodeIgniter core. They would additionally be used in *index.php*, but the constants file hasn't been loaded at that point, so the integer values are used instead, and a comment follows each such use with amplifying information on why that particular value was selected. Finally, the errors documentation has been updated accordingly. Signed-off-by: Daniel Hunsaker <danhunsaker@gmail.com>

danhunsaker · 2013-02-23T02:30:27Z

Alright, here it is. Finally got a chance to sit down and finish refining things. I think the end result is actually much better. Comments and (constructive) criticism requested. 😉

narfbg · 2013-03-04T07:03:27Z

application/config/constants.php

+ * CodeIgniter defaults
+ */
+define('EXIT_SUCCESS', 0); // no errors
+define('EXIT_FAILURE', 1); // generic error


A generic error should be named "ERROR", not "FAILURE". :)

Fair enough. This was probably a holdover from the stdlibc naming...

…e/exit-status

- Removed commented lists of constants from the three reference conventions, replacing each with the URLs at which more information can be found. - Renamed a few constants to more closely reflect CodeIgniter conventions. - Modified a couple of lines which were in violation of the CI Style Guide. Signed-off-by: Daniel Hunsaker <danhunsaker@gmail.com>

narfbg · 2013-03-04T09:14:41Z

system/core/CodeIgniter.php

@@ -188,7 +188,7 @@
 	if ($EXT->call_hook('cache_override') === FALSE
 		&& $OUT->_display_cache($CFG, $URI) === TRUE)
 	{
-		exit;


There are a few more occurences of this one ... :)

Signed-off-by: Daniel Hunsaker <danhunsaker@gmail.com>

narfbg · 2013-03-04T11:56:29Z

system/helpers/download_helper.php

@@ -141,7 +141,8 @@ function force_download($filename = '', $data = '', $set_mime = FALSE)
 		// If we have raw data - just dump it
 		if ($data !== NULL)
 		{
-			exit($data);


This is an HTTP-specific function, we don't need to echo & exit separately.

Hrm. I may have been overzealous in my desire for consistency.

…I apps to begin with Signed-off-by: Daniel Hunsaker <danhunsaker@gmail.com>

Updated all CLI-valid cases of exit() to return a valid code

The core shouldn't depend on constants that are not defined by itself

- Removed commented lists of constants from the three reference conventions, replacing each with the URLs at which more information can be found. - Renamed a few constants to more closely reflect CodeIgniter conventions. - Modified a couple of lines which were in violation of the CI Style Guide. Signed-off-by: Daniel Hunsaker <danhunsaker@gmail.com>

danhunsaker added 2 commits February 22, 2013 13:49

Merge branch 'develop' of github.com:EllisLab/CodeIgniter into featur…

44a6d1d

…e/exit-status

narfbg reviewed Mar 4, 2013
View reviewed changes

danhunsaker added 2 commits March 4, 2013 02:04

Merge branch 'develop' of github.com:EllisLab/CodeIgniter into featur…

e5d7af5

…e/exit-status

narfbg reviewed Mar 4, 2013
View reviewed changes

Oops, missed a few places where EXIT_SUCCESS was being used.

b2ac67a

Signed-off-by: Daniel Hunsaker <danhunsaker@gmail.com>

narfbg reviewed Mar 4, 2013
View reviewed changes

Reverting changes to functions that have no business being used in CL…

8626e93

…I apps to begin with Signed-off-by: Daniel Hunsaker <danhunsaker@gmail.com>

narfbg added a commit that referenced this pull request Mar 4, 2013

Merge pull request #2190 from danhunsaker/feature/exit-status

bb5cf0a

Updated all CLI-valid cases of exit() to return a valid code

narfbg merged commit bb5cf0a into bcit-ci:develop Mar 4, 2013

nonchip pushed a commit to nonchip/CodeIgniter that referenced this pull request Jun 29, 2013

Merge pull request bcit-ci#2190 from danhunsaker/feature/exit-status

857f4aa

Updated all CLI-valid cases of exit() to return a valid code

narfbg added a commit that referenced this pull request Mar 13, 2014

Partially revert PR #2190

7cf682a

The core shouldn't depend on constants that are not defined by itself

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated all CLI-valid cases of exit() to return a valid code #2190

Updated all CLI-valid cases of exit() to return a valid code #2190

danhunsaker commented Jan 25, 2013

AkenRoberts commented Jan 25, 2013

danhunsaker commented Jan 25, 2013

AkenRoberts commented Jan 25, 2013

danhunsaker commented Jan 25, 2013

AkenRoberts commented Jan 25, 2013

danhunsaker commented Jan 26, 2013

jbrower commented Jan 26, 2013

danhunsaker commented Feb 23, 2013

narfbg Mar 4, 2013

danhunsaker Mar 4, 2013

narfbg Mar 4, 2013

narfbg Mar 4, 2013

danhunsaker Mar 4, 2013

Updated all CLI-valid cases of exit() to return a valid code #2190

Updated all CLI-valid cases of exit() to return a valid code #2190

Conversation

danhunsaker commented Jan 25, 2013

AkenRoberts commented Jan 25, 2013

danhunsaker commented Jan 25, 2013

AkenRoberts commented Jan 25, 2013

danhunsaker commented Jan 25, 2013

AkenRoberts commented Jan 25, 2013

danhunsaker commented Jan 26, 2013

jbrower commented Jan 26, 2013

danhunsaker commented Feb 23, 2013

narfbg Mar 4, 2013

Choose a reason for hiding this comment

danhunsaker Mar 4, 2013

Choose a reason for hiding this comment

narfbg Mar 4, 2013

Choose a reason for hiding this comment

narfbg Mar 4, 2013

Choose a reason for hiding this comment

danhunsaker Mar 4, 2013

Choose a reason for hiding this comment