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

Already on GitHub? Sign in to your account

Driver extension enhancement #1787

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

dchill42 commented Sep 12, 2012

Followup to #353, and parallel fix to #1769.

Enables mix-and-match extension of Driver library and child classes. Extend library without requiring child extensions and vice-versa.

Also allows legacy Session library extensions to be applied to new Session driver without relocation to Session subdirectory.

Session and Cache drivers updated to work with changes to valid_drivers list, which now lists child class names without parent class name included. Driver child class and file names still require parent name prefix.

dchill42 added some commits Sep 10, 2012

@dchill42 dchill42 Started Driver extension changes
Signed-off-by: dchill42 <dchill42@gmail.com>
28bee3d
@dchill42 dchill42 Finished Driver extension changes
Signed-off-by: dchill42 <dchill42@gmail.com>
b9981f6

Round about here do something like this instead:

<?php

            if (! class_exists('CI_Driver_Library'))
            {
                // We aren't instantiating an object here, just making the base class available
                require BASEPATH.'libraries/Driver.php';
            }

Which is where it's going to matter and helps to make it understandable because the dependents are being included in the following two lines.

Owner

dchill42 replied Sep 12, 2012

True - with the changes to subdirectory handling, the Driver base loading up at L914 won't work anymore. I'll have to move that - just not to where you've indicated.

Is there any reason why include is not sufficient?
If yes rather do if class_exists as it's got cache advantages (ie APC) which *_once misses out on.

Owner

dchill42 replied Sep 12, 2012

Agreed - include is sufficient. include_once was the existing call, but it could be updated while we're here.

Something must be wrong, why are we going through the whole ric-ma-role again the class was included. Rather find out why it's doing double runs then to go the safe route here.

Owner

dchill42 replied Sep 12, 2012

There are two different operations here. The first pass loads an extension and the base class, whereas the second pass is for identifying the regular library in the package paths if an extension wasn't found. This is how the loader works.

nickl- commented on b9981f6 Sep 12, 2012

This is bad for cherry picking too much happening in one commit.

Can this not be don in 4 maybe even 8 separate commits?

I lost the plot when the cache changes started =(

Contributor

dchill42 commented Sep 12, 2012

I really don't think the number of commits is important. Just look at the diff on the pull request if you want to inspect the code changes. They are all interdependent and necessary to make this solution work.

Cache is the other library using the Driver class at this point (in addition to Session). I explained the change to Cache in the introductory comments on this pull request.

Contributor

dchill42 commented Sep 12, 2012

@narfbg and @alexbilbie - Here's my rewrite of #1769, now with Driver unit tests. I'd like to add to the Loader unit test to cover the changes there, but I'd prefer to do that after #1744 is merged, which sets the stage with basic CI_Loader::driver testing.

Contributor

toopay commented Sep 13, 2012

👍

@narfbg narfbg and 1 other commented on an outdated diff Sep 14, 2012

system/core/Loader.php
- // Check for match and driver base class
- if (strtolower(trim($subdir, '/')) == 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';
- }
+ // Establish subdirectories to try - if one was specified, just use that
+ $trysubs = array($subdir);
+ if ($subdir == '')
+ {
+ // Also check subdirectories matching class name
+ $dir = strtolower($class).'/';
+ $trysubs[] = ucfirst($dir);
+ $trysubs[] = $dir;
@narfbg

narfbg Sep 14, 2012

Contributor

I'm not really sure what this pull request does at all, but as a start ... indentation on this like looks weird. Additionaly - this particular element should not be added on Windows servers as they would ignore the case. Try this, with some comment added above it:

if (DIRECTORY_SEPARATOR !== '\\')
{
    $trysubs[] = $dir;
}
@dchill42

dchill42 Sep 14, 2012

Contributor

The change here handles three things - the lowercase "last ditch" that used to be below, the ucfirst "last, last ditch" that was added after that to pick up drivers with a library() call, and now the loading of an extension for a driver library from the main libs dir. Example - Session is now in libraries/Session/Session.php, this would allow an extension as libraries/MY_Session.php. The point is for extensions of the old Session library to be applied to the new Session driver after an upgrade without requiring relocation (drop-in replacement). The flexibility would apply to any driver library.

I'll check the indentation, but I know github is funny about it sometimes - the same tabs result in a different spacing in the diffs here. I suspect you mean L921 above, which is a classic example of that.

I agree that the case-sensitive variation is not needed on Windows, so the second addition is superfluous there. In the case of extension not found, that would skip an extra file_exists() test on that platform.

@narfbg narfbg and 1 other commented on an outdated diff Sep 14, 2012

system/core/Loader.php
- $is_duplicate = TRUE;
- log_message('debug', $class.' class already loaded. Second attempt ignored.');
- return;
- }
+ if ( ! $found)
+ {
+ // No base matches extension - bail
+ $msg = 'Unable to load the requested class: '.$class;
+ log_message('error', $msg);
+ show_error($msg);
+ }
+
+ // Does this look like a driver?
+ $driversub = strtolower($class).'/';
+ if ((strtolower($dir) == $driversub || strtolower($basesub) == $driversub)
+ && ! class_exists('CI_Driver_Library'))
@narfbg

narfbg Sep 14, 2012

Contributor

Even though all of those changes are obviously related to the Driver library, including checks about it in another class is a bad practice and shouldn't be done.

Also, the style guide says that you must use OR instead of || and line 958 should be further indented. Using === instead of == would also be nice. :)

@dchill42

dchill42 Sep 14, 2012

Contributor

This bit of code used to be up in the driver() call, but was moved down here in #353 to support loading driver libraries with library(). With this change, that check to include the driver base classes needed to move a little deeper into _ci_load_class() for the cases when we find an extension without the subdirectory for a driver lib. In short, this code has always been in Loader, it's just been relocated.

I know the style guide says OR, but I swear I've been seeing trending the other direction. I had come to assume that was a convention that had been reversed. If OR is still the true standard, OR it shall be, and yes - the explicit comparison will be better.

@narfbg narfbg commented on an outdated diff Sep 14, 2012

system/core/Loader.php
+ require BASEPATH.'libraries/Driver.php';
+ }
+
+ // 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))
+ {
+ $CI =& get_instance();
+ if ( ! isset($CI->$object_name))
+ {
+ return $this->_ci_init_class($class, config_item('subclass_prefix'), $params,
+ $object_name);
@narfbg

narfbg Sep 14, 2012

Contributor

Either put it on the line above, or put it more indentation and do the same to the rest of the method parameters.

@narfbg narfbg and 1 other commented on an outdated diff Sep 14, 2012

system/core/Loader.php
- include_once($baseclass);
- include_once($subclass);
- $this->_ci_loaded_files[] = $subclass;
+ $is_duplicate = TRUE;
+ log_message('debug', $class.' class already loaded. Second attempt ignored.');
+ return;
+ }
+
+ include($baseclass);
+ include($subclass);
@narfbg

narfbg Sep 14, 2012

Contributor

require_once()

@dchill42

dchill42 Sep 14, 2012

Contributor

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?

@narfbg

narfbg Nov 14, 2012

Contributor

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

@dchill42

dchill42 Nov 16, 2012

Contributor

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.

@narfbg

narfbg Nov 16, 2012

Contributor

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

@dchill42

dchill42 Nov 24, 2012

Contributor

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.

@narfbg

narfbg Nov 24, 2012

Contributor

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.

@narfbg narfbg commented on an outdated diff Sep 14, 2012

system/libraries/Driver.php
if ( ! isset($this->lib_name))
{
- $this->lib_name = get_class($this);
+ // Get library name without any prefix
+ $this->subclass_prefix = (string)$CI->config->item('subclass_prefix');
@narfbg

narfbg Sep 14, 2012

Contributor

Put spaces after type casts.

Contributor

narfbg commented Nov 14, 2012

@dchill42 Could you update this one?

Edit:

I don't know how appropriate it is to support legacy MY_Session implementations. Could you also say what your thoughts are on this?

Contributor

dchill42 commented Nov 16, 2012

Sure. I'm actually on the fence about the legacy MY_Session extensions. The reason I included code here to accept them is that I've made every other effort to ensure the Session driver is a fully backwards-compatible replacement for the former library. It seemed senseless to have one outstanding exception preventing us from saying "the default Session driver will work with your existing application with no changes required."

On the other hand, this is a major release and we do have a full changelog. It is not totally unreasonable to ask the (relatively few) developers who may have extended Session to move their extension into a subdirectory. It is also noteworthy that some of the undocumented non-API functions of the former library are gone. If an extension overloaded sess_update(), for example, it would no longer be called, as that function became _sess_update() in the driver. So, extensions probably need to be inspected for compatibility as we cannot absolutely guarantee their backward compatibility.

I guess I basically just talked myself into removing support for extensions outside the driver subdirectory and replacing it with a changelog note telling devs that MY_Session will have to be relocated and possibly retooled for the new driver.

Contributor

narfbg commented Nov 22, 2012

I don't think that we should support the old extensions. Anybody that has made changes to how CI behaves (be it by directly changing the code or by extending it) should read the upgrade notes and adjust their changes. Plus, as you noted - significant changes that could break old extensions has been made to the library anyway.

Just focus it on having the ability to extend drivers.

@narfbg narfbg commented on the diff Nov 24, 2012

system/core/Loader.php
@@ -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
@narfbg

narfbg Nov 24, 2012

Contributor

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

Contributor

dchill42 commented Nov 25, 2012

Replaced by #2029

@dchill42 dchill42 closed this Nov 25, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment