Re: Silenced include(_once) calls

I think this also need to be balanced against the concept, a) is it a setup/programming error b) is it a result of user-input If b) is likely, then this approach is correct, if a) is likely, then just using include_once, and displaying the error is probably more usefull. Regards Alan Justin Patrin wrote:

A user reported a problem with DB_DataObject_FormBuilder today that caused me to go back and look at the factory code in the create() function. Just yesterday I checked a new factory method into Text_Wiki so I had a fresh perspective on the factory algorithm. I traced the user's problem to a silenced include_once that I was erroneously using to check for file existance as well as include a file. Since I had just fixed this I decided to see how many other files in PEAR silence include calls. I was quite shocked to find that there are over 100 silenced include calls in PEAR code. http://pear.reversefold.com/include_once_silenced.txt I would like to make the case for this to be disallowed from PEAR code as part of the PEAR CS. The problem is that include_once has side effects and silencing the call causes lots of lots development time when things happen to go wrong. Consider the following error cases: * The file to include cannot be found. This is generally the case that these silenced calls are meant to deal with and it deals with it fine. The call returns false and the code can raise an error. * The include file is found but has a parse error The script dies on the spot without any output. This is strange as normally PHP will output when a parse error happens. This causes a major problem as the developer has to trace into all the code to find out what the problem is. Once it is found that the include_once is where it dies, removing the @ gives the developer the real error. * The file to include exists and has a require or require_once which cannot be found See above. The script will die but not output anything. This also applies recursively as required scripts could in turn require other scripts. * The file to include exists and has an include or require which includes a script with parse error Again, see above. The script will die but not output anything. Also a recursive problem. * The file to include parses fine but has a "return false;" or similar at the end include returns false even though the code was included, parsed, and run fine. This is not likely to ever be the case in actual PEAR code, but it could be the case in some maliciously crafted code or in someone's badly written code for their own use. It should be clear from these cases that checking for the return value of include is not a correct check for the includability of a file. In addition, it has many bad side-effects which cause developers to lose time debugging when they shouldn't have to. There are several alternatives to silencing include: 1) Leave it as-is but without @. This will cause PHP to display errors when files can't be found, but this is in general something that the developer wanted to see anyway. Remember that production websites should normally have error_reporting turned off and output to a file so that the user doesn't see it. 2) Split the include_path and check file_exists and is_readable for the file for each path. This is a solution which adds some extra cycles to the include process but allows the application to gracefully return an error when a file is not includable without a PHP error getting triggered. 3) Use @fopen($file, 'r', true). This is faster than #2 and still checks the include_path. This also does a check for readability in one call. This does use @, but in this case it has no horrible side-effects, such as the script dying. There are a few side-effects in that a registered PHP error handler will catch errors from this, but that is not likely to hurt an application in any way. This solution also allows an application to gracefully return an error without PHP errors being displayed. -- Justin Patrin