Cache Library Clean-up #3804
Cache Library Clean-up #3804
Conversation
- Comments! - Updates the cache library to validate *both* adapters. - No longer attempts to set an undefined "memcached" class variable. - $key variable renamed to $driver_type (more descriptive).
…vor of a simple if condition.
| ); | ||
|
|
||
| foreach ($default_config as $key) | ||
| if (isset($config['adapter']) && in_array($config['adapter'], $this->valid_drivers)) |
There was a problem hiding this comment.
This will silently ignore any invalid driver and keep the library working with the 'dummy' one.
There was a problem hiding this comment.
Good point. I've added an "error" level log_message call for invalid adapters, both primary and back-up. Let me know if you had something else in mind.
There was a problem hiding this comment.
I did ... you shouldn't have added the in_array() condition in the first place. Validation of the driver is done later.
There was a problem hiding this comment.
It was already there for the back-up adapter, I just added it for the primary adapter as well.
Validation of the driver is done later.
I assume by that you mean the is_supported function? That doesn't currently validate that the user has set a valid adapter, just that the adapter that's set is usable; is memcached actually installed? Taking the in_array() checks against the $valid_drivers array out would allow a user to set the adapter to one that doesn't exist causing an error when the is_supported adapter function is called on L266.
What are your thoughts on scrapping the $valid_drivers array and rewriting the is_supported function to fail gracefully if the adapter doesn't exist? These conditions would give the user fair warning that something is not correct. It's simple and arguably easier to add a custom written adapter.
There was a problem hiding this comment.
The conditions that you linked in your last paragraph are what makes the validity checks ... the logic that you're now introducing already exists in there - that's what I meant with my previous comment.
As for the $valid_drivers array ... maybe, I'm not really happy with it being user-configurable.
|
I'm still getting accustomed to the CI core... I can see now that the driver validation is already happening (for both primary and back-up drivers) in the system Driver library. This pull:
Let me know if there's anything else needed. |
|
@narfbg Any idea why the build is failing? |
|
Unrelated ... Travis is having some issues with MySQL. |
Continuation of #3800. I accidentally deleted the branch and GitHub won't update the old PR anymore.
This reverts the previous variable renaming and removes the foreach loop in favour of an if condition.