Using a class to create a custom wrapper for including files

Question

I am using PHP in order to create a custom wrapper for including files, for this I have written this class:

<?php
class Refrences
{
 #region properties
 private $_basePath;
 #endregion
 #region public members
 public function __construct() { $this->_basePath = $this->cleanDirName(dirname(__FILE__)); }
 public function includeFile($f) { $this->ior(1, $f); }
 public function requireFile($f) { $this->ior(2, $f); }
 public function includeFileOnce($f) { $this->ior(3, $f); }
 public function requireFileOnce($f) { $this->ior(4, $f); }
 public function includeArray(array $files)
 {
 foreach ($files as $f) { $this->ior(1, $f); }
 }
 public function requireArray(array $files)
 {
 foreach ($files as $f) { $this->ior(2, $f); }
 }
 public function includeArrayOnce(array $files)
 {
 foreach ($files as $f) { $this->ior(3, $f); }
 }
 public function requireArrayOnce(array $files)
 {
 foreach ($files as $f) { $this->ior(4, $f); }
 }
 #endregion
 #region private members
 private function cleanDirName($f) { return rtrim(str_replace("\\", "/", dirname($f)), "/") . "/"; }
 private function fileExists($f) { return (!file_exists($f)) ? false : true; }
 private function isFile($f) { return (is_dir($f) || !is_file($f)) ? false: true; }
 private function ior($switch, $file)
 {
 switch ($switch)
 {
 case 1:
 if ($this->checkIsFile($file)) include ($file);
 else die("Cannot load file.");
 break;
 case 2:
 if ($this->checkIsFile($file)) require ($file);
 else die("Cannot load file.");
 break;
 case 3:
 if ($this->checkIsFile($file)) include_once ($file);
 else die("Cannot load file.");
 break;
 case 4:
 if ($this->checkIsFile($file)) require_once ($file);
 else die("Cannot load file.");
 break;
 }
 }
 private function checkIsFile($f) { return ($this->fileExists($f) && $this->isFile($f)) ? true : false; }
 #endregion
}

And all functionality etc. works, but is this a good thing to do? Does it allow me greater control over the files that are included? Is there a better way to do this?

Answer 1

• don't shorten method or variable names. It's really hard to see what ior means. I'm guessing includeOrRequire?
• You should definitely check for directory traversal. You have the base name and the file name, so you can check if the file is actually in the base dir. You could also additionally do some sanitation on base name (eg replace ..).
• Your cleanDirName replaces \ with /. Why not replace / and \ with DIRECTORY_SEPARATOR, so it works independent of the OS?
• You shouldn't die in functions, as it makes it impossible for the calling code to recover (or even to display a custom error page). It also makes the distinction between include and require meaningless, as the only difference is that one holds execution while the other doesn't
• return cond ? true : false can be written as return cond.
• Don't put the method content in the same line as the method signature, it makes your code less readable. Same for foreach, etc.
• Having the ior function doesn't actually provide any benefits. It doesn't remove any duplication, and it introduces extra complexity (the cases are also rather random, just looking at $this->ior(2, $f);, I would have no idea what it does).
• fileExists is unnecessary, its just a rename of file_exists. It's also written confusingly, because of the unnecessary negation.
• I don't see why you need to check if is_dir and if is_file. Either it's one or the other. Just is_file would be enough. And then your isFile method is also unnecessary, as it's just a rename of is_file.
• You don't use basepath, so why have it in the constructor?

Taking all this (without the validation, you would need to add that), you would get:

<?php
class Refrences
{
 public function __construct() { }
 public function includeFile($file) { 
 if ($this->isExistingFile($file)) {
 include($file); 
 } else {
 // throw exception or return false.
 }
 }
 // [...]
 public function includeArray(array $files)
 {
 foreach($files as $file) {
 includeFile($file);
 }
 }
 // [...]
 private function cleanDirName($file) { 
 return rtrim(str_replace(["\\", "/"], DIRECTORY_SEPARATOR, dirname($file)), "/") . "/"; 
 }
 private function isExistingFile($file) { 
 return file_exists($file) && is_file($file);
 }
}

Without the validation for directory traversal, this is basically just a renaming of include, etc to includeFile. I'm not sure it's worth it. You do save the file exists and is file checks, but you have to check the return of the include call anyways.

It does however give you one central place where all includes take place, so if you do want to do additional stuff when including (like checking for directory traversal or normalizing paths for different OSes), it wouldn't be bad to have code like this.

• anon: Thank you for this, a lot to look at here and take into account...