Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Driver extension enhancement #1787

Closed
wants to merge 11 commits into from
Closed

Driver extension enhancement #1787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
28bee3d
Started Driver extension changes
dchill42 Sep 10, 2012
b9981f6
Finished Driver extension changes
dchill42 Sep 11, 2012
1db3e0e
Tweaked Driver base autoload and switched include_once to include
dchill42 Sep 12, 2012
0fc5320
Added Driver unit test
dchill42 Sep 12, 2012
dd887a2
Merge branch 'develop' of github.com:/EllisLab/CodeIgniter into session
dchill42 Sep 12, 2012
9576deb
Merge branch 'develop' of github.com:/EllisLab/CodeIgniter into session
dchill42 Sep 14, 2012
9caf878
Cleanup
dchill42 Sep 14, 2012
f0e2924
Merge branch 'develop' of github.com:/EllisLab/CodeIgniter into session
dchill42 Oct 9, 2012
52bcffb
Merge branch 'develop' of git://github.com/EllisLab/CodeIgniter into …
dchill42 Nov 24, 2012
5af440d
Merge branch 'session' of github.com:dchill42/CodeIgniter into session
dchill42 Nov 24, 2012
b1ddeb0
Removed support for driver extensions in libraries dir and fixed sess…
dchill42 Nov 24, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 100 additions & 72 deletions system/core/Loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* Licensed under the Open Software License version 3.0
*
* This source file is subject to the Open Software License (OSL 3.0) that is
* bundled with this package in the files license.txt / license.rst. It is
* bundled with this package in the files license.txt / license.rst. It is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert those, I think we already discussed them on another PR.

* also available through the world wide web at this URL:
* http://opensource.org/licenses/OSL-3.0
* If you did not receive a copy of the license and are unable to obtain it
Expand Down Expand Up @@ -136,7 +136,7 @@ class CI_Loader {
*/
public function __construct()
{
$this->_ci_ob_level = ob_get_level();
$this->_ci_ob_level = ob_get_level();
$this->_ci_library_paths = array(APPPATH, BASEPATH);
$this->_ci_helper_paths = array(APPPATH, BASEPATH);
$this->_ci_model_paths = array(APPPATH);
Expand Down Expand Up @@ -949,109 +949,137 @@ protected function _ci_load_class($class, $params = NULL, $object_name = NULL)

// Get the filename from the path
$class = substr($class, $last_slash);
}

// Check for match and driver base class
if (strtolower(trim($subdir, '/')) == strtolower($class) && ! class_exists('CI_Driver_Library'))
// Establish subdirectories to try - if one was specified, just use that
$trysubs = array($subdir);
if ($subdir === '')
{
// Also check subdirectories matching class name
// This allows us to find libraries in self-named subdirectories
// (like Drivers) and extensions for those in the main libraries dir
$dir = strtolower($class).'/';
$trysubs[] = ucfirst($dir);

// If we aren't case-insensitive, check lowercase too
if (DIRECTORY_SEPARATOR !== '\\')
{
// We aren't instantiating an object here, just making the base class available
require BASEPATH.'libraries/Driver.php';
$trysubs[] = $dir;
}
}

// Get the subclass prefix
$prefix = config_item('subclass_prefix');

// We'll test for both lowercase and capitalized versions of the file name
foreach (array(ucfirst($class), strtolower($class)) as $class)
$cases = array(ucfirst($class));
if (DIRECTORY_SEPARATOR !== '\\')
{
// Case-sensitive - try lowercase too
$cases[] = strtolower($class);
}
foreach ($cases as $class)
{
$subclass = APPPATH.'libraries/'.$subdir.config_item('subclass_prefix').$class.'.php';

// Is this a class extension request?
if (file_exists($subclass))
foreach($trysubs as $dir)
{
$baseclass = BASEPATH.'libraries/'.ucfirst($class).'.php';

if ( ! file_exists($baseclass))
// Check each path for an extension file
$subclass = APPPATH.'libraries/'.$dir.$prefix.$class.'.php';
if (file_exists($subclass))
{
log_message('error', 'Unable to load the requested class: '.$class);
show_error('Unable to load the requested class: '.$class);
}
// Require the base class in the same directory
$baseclass = BASEPATH.'libraries/'.$dir.ucfirst($class).'.php';
if ( ! file_exists($baseclass))
{
// No base matches extension - bail
$msg = 'Unable to load the requested class: '.$class;
log_message('error', $msg);
show_error($msg);
}

// Safety: Was the class already loaded by a previous call?
if (in_array($subclass, $this->_ci_loaded_files))
{
// Before we deem this to be a duplicate request, let's see
// if a custom object name is being supplied. If so, we'll
// return a new instance of the object
if ( ! is_null($object_name))
// Does this look like a driver?
if (strtolower($dir) === strtolower($class).'/' && ! class_exists('CI_Driver_Library'))
{
// We aren't instantiating an object here, just making the base class available
require BASEPATH.'libraries/Driver.php';
}

// Safety: Was the class already loaded by a previous call?
if (in_array($subclass, $this->_ci_loaded_files))
{
$CI =& get_instance();
if ( ! isset($CI->$object_name))
// Before we deem this to be a duplicate request, let's see
// if a custom object name is being supplied. If so, we'll
// return a new instance of the object
if ( ! is_null($object_name))
{
return $this->_ci_init_class($class, config_item('subclass_prefix'), $params, $object_name);
$CI =& get_instance();
if ( ! isset($CI->$object_name))
{
return $this->_ci_init_class($class, $prefix, $params, $object_name);
}
}
}

$is_duplicate = TRUE;
log_message('debug', $class.' class already loaded. Second attempt ignored.');
return;
}
$is_duplicate = TRUE;
log_message('debug', $class.' class already loaded. Second attempt ignored.');
return;
}

include_once($baseclass);
include_once($subclass);
$this->_ci_loaded_files[] = $subclass;
include($baseclass);
include($subclass);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require_once()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll change this if you say so, but I don't see the point in require_once for these files. The Loader already tracks what's been loaded, and we've already verified both files exist. Am I missing some reason for having PHP do the extra work of require_once over include in this scenario?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple - it is way better to leave to job to PHP instead of trying to track it yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just it, though - Loader has always tracked loaded classes internally in order to recognize duplicate requests and either skip the include to instantiate another object under a different name, or gracefully return on true duplicates after providing a log message to inform the developer of the redundant call. If we remove that legacy logic and just rely on require_once, we will lose that courtesy. So, which way do we want to go - internal tracking or require_once? I don't see any point in doing both - it's just wasteful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Save library names in a property, leave PHP to handle file includes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, last push on this - if you insist, I'll make those require_once() even though I disagree. The difference between include() and require_once() is making sure the file exists and hasn't already been included. If the library name isn't already in the property, we know the file hasn't been included yet. If we found the file in the current directory, we already know it exists. These are steps we have to take in our loading process. So, why have PHP repeat those same checks? It's redundant and inefficient. As I said above, if you insist on that redundancy, I'll make the change and shut up about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My whole point is - I don't get the whole idea of why do we have to have a _ci_loaded_files array, fill it, check against it, etc. when we can just use include_once() or require_once(). Those functions WILL always be faster than whatever tracking we implement on our own.

On a side note - I don't really care if it is require or include.

$this->_ci_loaded_files[] = $subclass;

return $this->_ci_init_class($class, config_item('subclass_prefix'), $params, $object_name);
return $this->_ci_init_class($class, $prefix, $params, $object_name);
}
}

// Lets search for the requested library file and load it.
$is_duplicate = FALSE;
foreach ($this->_ci_library_paths as $path)
{
$filepath = $path.'libraries/'.$subdir.$class.'.php';

// Does the file exist? No? Bummer...
if ( ! file_exists($filepath))
foreach ($trysubs as $dir)
{
continue;
}
$filepath = $path.'libraries/'.$dir.$class.'.php';

// Safety: Was the class already loaded by a previous call?
if (in_array($filepath, $this->_ci_loaded_files))
{
// Before we deem this to be a duplicate request, let's see
// if a custom object name is being supplied. If so, we'll
// return a new instance of the object
if ( ! is_null($object_name))
// Does the file exist? No? Bummer...
if ( ! file_exists($filepath))
{
continue;
}

// Does this look like a driver?
if (strtolower($dir) === strtolower($class).'/' && ! class_exists('CI_Driver_Library'))
{
// We aren't instantiating an object here, just making the base class available
require BASEPATH.'libraries/Driver.php';
}

// Safety: Was the class already loaded by a previous call?
if (in_array($filepath, $this->_ci_loaded_files))
{
$CI =& get_instance();
if ( ! isset($CI->$object_name))
// Before we deem this to be a duplicate request, let's see
// if a custom object name is being supplied. If so, we'll
// return a new instance of the object
if ( ! is_null($object_name))
{
return $this->_ci_init_class($class, '', $params, $object_name);
$CI =& get_instance();
if ( ! isset($CI->$object_name))
{
return $this->_ci_init_class($class, '', $params, $object_name);
}
}

$is_duplicate = TRUE;
log_message('debug', $class.' class already loaded. Second attempt ignored.');
return;
}

$is_duplicate = TRUE;
log_message('debug', $class.' class already loaded. Second attempt ignored.');
return;
include($filepath);
$this->_ci_loaded_files[] = $filepath;
return $this->_ci_init_class($class, '', $params, $object_name);
}

include_once($filepath);
$this->_ci_loaded_files[] = $filepath;
return $this->_ci_init_class($class, '', $params, $object_name);
}
} // END FOREACH

// One last attempt. Maybe the library is in a subdirectory, but it wasn't specified?
if ($subdir === '')
{
$path = strtolower($class).'/'.$class;
return $this->_ci_load_class($path, $params, $object_name);
}
elseif (ucfirst($subdir) != $subdir)
{
// Lowercase subdir failed - retry capitalized
$path = ucfirst($subdir).$class;
return $this->_ci_load_class($path, $params, $object_name);
}

// If we got this far we were unable to find the requested class.
// We do not issue errors if the load call failed due to a duplicate request
if ($is_duplicate === FALSE)
Expand Down Expand Up @@ -1324,4 +1352,4 @@ protected function _ci_prep_filename($filename, $extension)
}

/* End of file Loader.php */
/* Location: ./system/core/Loader.php */
/* Location: ./system/core/Loader.php */
14 changes: 7 additions & 7 deletions system/libraries/Cache/Cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ class CI_Cache extends CI_Driver_Library {
*
* @var array
*/
protected $valid_drivers = array(
'cache_apc',
'cache_dummy',
'cache_file',
'cache_memcached',
'cache_redis',
'cache_wincache'
protected $valid_drivers = array(
'apc',
'dummy',
'file',
'memcached',
'redis',
'wincache'
);

/**
Expand Down
Loading