Permalink
Browse files

Improvements to Loader allow multiple instances.

By removing ALL $is_duplicate checking from the Loader class we can do two things:

1. Remove reliance on $this->foo, which makes passing classes as arguments considerably easier.

2. No more singleton! When loaded once CodeIgniter will assign the first item to $this->foo as always, which should be considered deprecated. Now it can "load" a library multiple times and have the instance returned from the loader.

Eg: 
    $instance1 = $this->load->library('table');
    $instance2 = $this->load->library('table');

Previously, library would do nothing and model would error (although this has not yet been implemented for models).

Feedback on a postcard.
  • Loading branch information...
Phil Sturgeon
Phil Sturgeon committed May 14, 2012
1 parent fff6c2a commit 64a2e853bc83740f0802a29e5efccd52ae7636eb
Showing with 74 additions and 83 deletions.
  1. +40 −71 system/core/Loader.php
  2. +34 −12 tests/codeigniter/core/Loader_test.php
View
@@ -199,7 +199,7 @@ public function is_loaded($class)
* @param string an optional object name
* @return void
*/
- public function library($library = '', $params = NULL, $object_name = NULL)
+ public function library($library = '', $params = NULL, $rename_to = NULL)
{
if (is_array($library))
{
@@ -213,15 +213,10 @@ public function library($library = '', $params = NULL, $object_name = NULL)
if ($library == '' OR isset($this->_base_classes[$library]))
{
- return FALSE;
- }
-
- if ( ! is_null($params) && ! is_array($params))
- {
- $params = NULL;
+ return NULL;
}
- $this->_ci_load_class($library, $params, $object_name);
+ return $this->_ci_load_class($library, $params, $rename_to);
}
// --------------------------------------------------------------------
@@ -247,9 +242,9 @@ public function model($model, $name = '', $db_conn = FALSE)
return;
}
- if ($model == '')
+ if (empty($model))
{
- return;
+ return NULL;
}
$path = '';
@@ -275,6 +270,7 @@ public function model($model, $name = '', $db_conn = FALSE)
}
$CI =& get_instance();
+
if (isset($CI->$name))
{
show_error('The model name you are loading is the name of a resource that is already being used: '.$name);
@@ -628,7 +624,7 @@ public function config($file = '', $use_sections = FALSE, $fail_gracefully = FAL
* @param string an optional object name
* @return void
*/
- public function driver($library = '', $params = NULL, $object_name = NULL)
+ public function driver($library = '', $params = NULL, $rename_to = NULL)
{
if (is_array($library))
{
@@ -657,7 +653,7 @@ public function driver($library = '', $params = NULL, $object_name = NULL)
$library = ucfirst($library).'/'.$library;
}
- return $this->library($library, $params, $object_name);
+ return $this->library($library, $params, $rename_to);
}
// --------------------------------------------------------------------
@@ -901,7 +897,7 @@ protected function _ci_load($_ci_data)
* @param string an optional object name
* @return void
*/
- protected function _ci_load_class($class, $params = NULL, $object_name = NULL)
+ protected function _ci_load_class($class, $params = NULL, $rename_to = NULL)
{
// Get the class name, and while we're at it trim any slashes.
// The directory path can be included as part of the class name,
@@ -937,34 +933,17 @@ protected function _ci_load_class($class, $params = NULL, $object_name = NULL)
}
// Safety: Was the class already loaded by a previous call?
- if (in_array($subclass, $this->_ci_loaded_files))
+ 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))
- {
- $CI =& get_instance();
- if ( ! isset($CI->$object_name))
- {
- return $this->_ci_init_class($class, config_item('subclass_prefix'), $params, $object_name);
- }
- }
-
- $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_once($baseclass);
- include_once($subclass);
- $this->_ci_loaded_files[] = $subclass;
-
- return $this->_ci_init_class($class, config_item('subclass_prefix'), $params, $object_name);
+ return $this->_ci_init_class($class, config_item('subclass_prefix'), $params, $rename_to);
}
// 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';
@@ -976,46 +955,27 @@ protected function _ci_load_class($class, $params = NULL, $object_name = NULL)
}
// Safety: Was the class already loaded by a previous call?
- if (in_array($filepath, $this->_ci_loaded_files))
+ 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))
- {
- $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;
+ include $filepath;
+ $this->_ci_loaded_files[] = $filepath;
}
- include_once($filepath);
- $this->_ci_loaded_files[] = $filepath;
- return $this->_ci_init_class($class, '', $params, $object_name);
+ return $this->_ci_init_class($class, '', $params, $rename_to);
}
} // END FOREACH
// One last attempt. Maybe the library is in a subdirectory, but it wasn't specified?
- if ($subdir == '')
+ if ($subdir === '')
{
$path = strtolower($class).'/'.$class;
return $this->_ci_load_class($path, $params);
}
// 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)
- {
- log_message('error', 'Unable to load the requested class: '.$class);
- show_error('Unable to load the requested class: '.$class);
- }
+ log_message('error', 'Unable to load the requested class: '.$class);
+ show_error('Unable to load the requested class: '.$class);
}
// --------------------------------------------------------------------
@@ -1029,7 +989,7 @@ protected function _ci_load_class($class, $params = NULL, $object_name = NULL)
* @param string an optional object name
* @return void
*/
- protected function _ci_init_class($class, $prefix = '', $config = FALSE, $object_name = NULL)
+ protected function _ci_init_class($class, $prefix = '', $config = FALSE, $rename_to = NULL)
{
// Is there an associated config file for this class? Note: these should always be lowercase
if ($config === NULL)
@@ -1099,30 +1059,39 @@ protected function _ci_init_class($class, $prefix = '', $config = FALSE, $object
// Set the variable name we will assign the class to
// Was a custom class name supplied? If so we'll use it
- $class = strtolower($class);
+ $class_name = strtolower($class);
- if (is_null($object_name))
+ if (is_null($rename_to))
{
- $classvar = isset($this->_ci_varmap[$class]) ? $this->_ci_varmap[$class] : $class;
+ $classvar = isset($this->_ci_varmap[$class_name]) ? $this->_ci_varmap[$class_name] : $class_name;
}
else
{
- $classvar = $object_name;
+ $classvar = $rename_to;
}
// Save the class name and object name
- $this->_ci_classes[$class] = $classvar;
+ $this->_ci_classes[$class_name] = $classvar;
// Instantiate the class
- $CI =& get_instance();
if ($config !== NULL)
{
- $CI->$classvar = new $name($config);
+ $class = new $name($config);
}
else
{
- $CI->$classvar = new $name();
+ $class = new $name;
}
+
+ // DEPRECATED Future versions (3.1?) will no longer assign to the super-global
+ $CI =& get_instance();
+ if ( ! isset($CI->$classvar))
+ {
+ $CI->$classvar = $class;
+ }
+ // End DEPRECATED
+
+ return $class;
}
// --------------------------------------------------------------------
@@ -1263,4 +1232,4 @@ protected function _ci_prep_filename($filename, $extension)
}
/* End of file Loader.php */
-/* Location: ./system/core/Loader.php */
+/* Location: ./system/core/Loader.php */
@@ -21,17 +21,35 @@ public function set_up()
public function test_library()
{
$this->_setup_config_mock();
-
- // Test loading as an array.
+
+ // Test no lib given
+ $this->assertEquals(NULL, $this->load->library());
+
+ // Test loading multiple with an array.
$this->assertNull($this->load->library(array('table')));
+
+ // Does the table class exist now its been loaded
$this->assertTrue(class_exists('CI_Table'), 'Table class exists');
+
+ // Check the old (now deprecated) super-global assignment works
$this->assertAttributeInstanceOf('CI_Table', 'table', $this->ci_obj);
- // Test no lib given
- $this->assertEquals(FALSE, $this->load->library());
-
- // Test a string given to params
- $this->assertEquals(NULL, $this->load->library('table', ' '));
+ // Return the library instance
+ $email = $this->load->library('email');
+
+ // The factory created instance must be CI_Email
+ $this->assertInstanceOf('CI_Email', $email);
+
+ // Check the old deprecated approach works too
+ $this->assertInstanceOf('CI_Email', $this->ci_obj->email);
+
+ // Are those two objects entirely the same
+ $this->assertSame($email, $this->ci_obj->email);
+
+ $email2 = $this->load->library('email');
+
+ // Are those two objects entirely the same
+ $this->assertNotSame($email, $email2);
}
// --------------------------------------------------------------------
@@ -42,10 +60,14 @@ public function test_load_library_in_application_dir()
$content = '<?php class Super_test_library {} ';
- $model = vfsStream::newFile('Super_test_library.php')->withContent($content)
- ->at($this->load->libs_dir);
-
- $this->assertNull($this->load->library('super_test_library'));
+ vfsStream::newFile('Super_test_library.php')
+ ->withContent($content)
+ ->at($this->load->libs_dir);
+
+ // Return the library instance
+ $library = $this->load->library('super_test_library');
+
+ $this->assertInstanceOf('Super_test_library', $library);
// Was the model class instantiated.
$this->assertTrue(class_exists('Super_test_library'));
@@ -73,7 +95,7 @@ public function test_non_existent_model()
$this->setExpectedException(
'RuntimeException',
'CI Error: Unable to locate the model you have specified: ci_test_nonexistent_model.php'
- );
+ );
$this->load->model('ci_test_nonexistent_model.php');
}

19 comments on commit 64a2e85

Contributor

JonoB replied May 15, 2012

Does it still work if you specifically want a library to be loaded as a singleton?

Contributor

philsturgeon replied May 15, 2012

Any library can be loaded as a singleton. A singleton is just something that is used once and does not have multiple instances.So you can do any of this:

// New behaviour
$email = $this->load->library('email');

This allows you to use multiple instances.

// or old behaviour (deprecated)
$this->load->library('email');
$this->email->to('bla@foo.com');

This is how things have always worked and is still supported. The first time you load a library it will assign it to the super-global, meaning you need to clear it and only have the one instance. The second time you load a library it will NOT assign it to the super-global - but an instance has been created anyway and not captured in a variable which is a waste. In fairness if you load a class multiple times you should expect it to instantiate multiple times. This seems more logical than the current behaviour.

// or weird behaviour
$this->email = $this->load->library('email');
$this->email->to('bla@foo.com');

This allows you to assign classes to the super-instance, and will continue to work after deprecated code has been removed. It looks a little odd, but is just what CI was forcing in the background.

Contributor

JonoB replied May 15, 2012

Sorry, what I meant to say was "Does it still work if you specifically want a library to be loaded into the singleton?" (i.e. into the super instance). And the answer appears to be yes based on your third example.

That would be a significant amount of re-factoring...but worth it. For one, I can finally fix those stupid validation bugs (allowing form_validation to validate any array) that kept getting caught by the singleton ;)

Contributor

philsturgeon replied May 15, 2012

It would not so much be refactoring, but it would involve some tweaking. If you were to do "Find all $this->load->library" calls you could do this in 20 minutes, or just not bother until 3.1 and who knows when that will be.

The fact that this is fully backwards compatible but will require some tweaks on a x.0 version release will HOPEFULLY be ok with EllisLab and the rest of the community. We'll see what people say.

This is awesome

Contributor

benedmunds replied May 15, 2012

Definitely needed! +1

@ghost

Love the idea!

Just a question on the conversion. What is the suggested procedure to pass object instances from controllers to models? Like if you needed to get the current users_id and wanted to access $this->auth->users_id() in both the controller and model. Since that way is now deprecated, what is the proper way to pass those object references between other methods and objects?

Contributor

philsturgeon replied May 15, 2012

Generally accessing a shared property from one class by replicating $this is known as really bad coupling by most computer scientist types.

Would would be a much better idea is passing in the user_id, as this would make your code way more flexible.

If it IS an issue, you can still do $this->auth = $this->load->library('auth') and have it assigned to the super-global, and 3.0 will do that for you anyway.

Works the same, will eventually require a tweak, but generally you shouldnt be doing that anyway. Happy? :)

@ghost

Sounds good! Would this apply to the core classes too? For example, we shouldn't be accessing the URI object via $this->uri->uri_string()?

This is the behaviour i've been missing so sorely from CI. Love it.

Contributor

philsturgeon replied May 16, 2012

@Eclarian it's not quite as cut and dry as saying "using $this->foo is bad so never use it". For example in a Controller the uri is a property of the request, so realistically making it a property of the controller makes sense.

This is more about handling multiple image uploads, multiple curl requests, etc without having to "reset" the class. You should never need to say "I would like this class to go back to how it was when it was new", instead you should say "I am done with this instance, I'd like to use a different instance now" and this allows you to do that.

@ghost

@philsturgeon Awesome! I would love to never have to use $this->image_lib->clear() ever again. :)

Contributor

mpmont replied May 16, 2012

+1 on that @Eclarian =)

Glad to see this is coming. Just ran into a problem in a project today that required this. +1!

Contributor

onigoetz replied May 29, 2012

Just a question as you say in the code "DEPRECATED Future versions (3.1?) will no longer assign to the super-global"

Will there be a sort of "classes respository" ? or a global object that won't be the same than the controller ?

Or will we need to keep a variable for each and every object ?

Contributor

philsturgeon replied May 29, 2012

Contributor

onigoetz replied May 29, 2012

Yeah, Sure, but I have a lot of helpers that access CI Libraries, and doing $this->foo = $this->load->library('foo'); to add every library again is a bit of a regression from my point of view.

So my question is more what will be the best practice when this will be disabled ?

And to say "do manually what was done automatically before" doesn't sound correct to me :D

so my question / proposition : is it possible to make that get_instance() returns a repository of classes like it did now, and maybe get_controllers() to return loaded controllers (if needed ) ? because this is very useful !

I personally added a helper to have CI()->foo

and having a thing like CI()->foo instead of $this->foo is still readable. and others ways of doing it can become quite messy

Contributor

philsturgeon replied May 30, 2012

Using $this->foo = $this->load->library('foo') is only so you can continue to use "bad coupling" throughout CodeIgniter. I won't be doing it, and the only place it makes sense to me is to add $this->uri and $this->user_agent so they are relative to the controller instead of "a super instance that contains everything".

In your helpers you will want to work with instances, so you can use the loader to do that. If you want ONE to exist thoughout the entire instance then you assign it to $this as you are currently forced to do.

CI() or $this are irrelevant to this :)

Please sign in to comment.