zendframework / zend-cache

BSD 3-Clause "New" or "Revised" License
69 stars 53 forks source link

Typo in variable for ExtMongoDbResourceManager adapter makes passing db a no-op #181

Closed TysonAndre closed 5 years ago

TysonAndre commented 5 years ago

The latest commit on master uses isset($resouce), which is a typo (and an undeclared variable). So selectCollection is always passed 'zend' as the first argument (noticed when running codespell)

Attempting to fix this may break existing cache installations - I'm not sure what the impact is because I'm not familiar with use cases.

--- a/src/Storage/Adapter/ExtMongoDbResourceManager.php
+++ b/src/Storage/Adapter/ExtMongoDbResourceManager.php
@@ -98,7 +98,7 @@ class ExtMongoDbResourceManager
                 }

                 $collection = $resource['client_instance']->selectCollection(
-                    isset($resouce['db']) ? $resource['db'] : 'zend',
+                    isset($resource['db']) ? $resource['db'] : 'zend',
                     isset($resource['collection']) ? $resource['collection'] : 'cache'
                 );

Code to reproduce the issue

Creating resources with a non-default db (e.g. 'db1', 'db2') and managing them with this cache would instead get created in the 'zend' db when using ExtMongoDbResourceManager but not MongoDbResourceManager and unexpectedly affect what was intended to be a different db?

(I'm not familiar enough with this library to be sure of that)

Expected results

Actual results

TysonAndre commented 5 years ago

@weierophinney - Any ideas on what the impact of fixing the typo would be (e.g. would applications using this library lose data)?

weierophinney commented 5 years ago

They wouldn't lose data - but if they had configured the database name, they'd now be hitting a different database. If they want to use the original, they'd have to either copy records to the correct database, or change their configuration to point to the "zend" database.

Personally, I consider this a bugfix. We'd simply need to address the issue in the CHANGELOG with the above options when we release.

Could you submit a PR, please?

glennschmidt commented 5 years ago

Looking forward to this fix being pulled