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.

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

Driver extension enhancement #1787

Closed
wants to merge 11 commits into from
Closed

Conversation

dchill42
Copy link
Contributor

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.

Signed-off-by: dchill42 <dchill42@gmail.com>
Signed-off-by: dchill42 <dchill42@gmail.com>
@dchill42
Copy link
Contributor Author

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.

@dchill42
Copy link
Contributor Author

@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.

@toopay
Copy link
Contributor

toopay commented Sep 13, 2012

👍

// Also check subdirectories matching class name
$dir = strtolower($class).'/';
$trysubs[] = ucfirst($dir);
$trysubs[] = $dir;
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
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?

@dchill42
Copy link
Contributor Author

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.

@narfbg
Copy link
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.

@@ -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
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.

@dchill42
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants