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

Added couchbase storage #11

Merged
merged 5 commits into from Jan 27, 2013

Conversation

Projects
None yet
3 participants
Contributor

SimonSimCity commented Dec 3, 2012

Added a storage implementation for couchbase.

Or do you think that memcache would be enough, as it's a drop-in replacement?
http://www.couchbase.com/memcached

Contributor

SimonSimCity commented Dec 3, 2012

Just one question left ...

The interface defines the methods "insert" and "update". In Couchbase you have "add" which will return an error if the data-set exists, "replace" which will only save the data if the data-set exists and "set" which will save the data whether the key already exists or not.

I used "add" for "insert". Please write it down if that's wrong implemented.

@stof stof commented on an outdated diff Dec 3, 2012

.../Tests/KeyValueStore/Storage/CouchbaseStorageTest.php
+
+namespace Doctrine\Tests\KeyValueStore\Storage;
+
+use Doctrine\KeyValueStore\Storage\CouchbaseStorage;
+
+/**
+ * Couchbase storage testcase
+ *
+ * @author Simon Schick <simonsimcity@gmail.com>
+ */
+class CouchbaseStorageTest extends \PHPUnit_Framework_TestCase
+{
+ /**
+ * @var \PHPUnit_Framework_MockObject_MockObject
+ */
+ private $couchbase;
@stof

stof Dec 3, 2012

Member

Please use spaces for the indentation, not tabs

Member

stof commented Dec 3, 2012

The DBALStorage uses a SQL Insert so it will also fail on existing keys: https://github.com/doctrine/KeyValueStore/blob/master/lib/Doctrine/KeyValueStore/Storage/DBALStorage.php#L78
However, you should catch the exception to be consistent, and either discard it (behavior of the DBALStorage but probably bad for debugging) or wrap it in a StorageException (behavior of the WindowsAzureTableStorage). @beberlei what should be the storage behavior ?

and you should add a suggestion for ext-couchbase in the composer.json

@stof stof commented on an outdated diff Dec 3, 2012

.../Tests/KeyValueStore/Storage/CouchbaseStorageTest.php
+ */
+class CouchbaseStorageTest extends \PHPUnit_Framework_TestCase
+{
+ /**
+ * @var \PHPUnit_Framework_MockObject_MockObject
+ */
+ private $couchbase;
+
+ /**
+ * @var CouchbaseStorage
+ */
+ private $storage;
+
+ protected function setUp()
+ {
+ $this->couchbase = $this->getMockBuilder('\Couchbase')
@stof

stof Dec 3, 2012

Member

you need to skip the test when the Couchbase class is not available instead of making it fail with an exception

Contributor

SimonSimCity commented Dec 3, 2012

The current implementation will just do nothing as you don't get a return-value ... The php-extension returns FALSE if insert or update fails - but I don't know how to handle it ..

The current implementation for DBAL f.e. is quite loosy as well .. they all don't care if the functions fail.
Like it looks for me, this KeyValueStore's are just thought for caching ... It's not sure if your data really got saved.

Member

stof commented Dec 3, 2012

@SimonSimCity they are the storage, not a cache (except if you use the whole KeyValueStore as a cache)

Contributor

SimonSimCity commented Dec 4, 2012

@stof: I know that ... but every implementation in here is build this way, that if an error occurs, f.e. during adding (key already exists ..), it's just ignored.
But that's another discussion ;)

beberlei added a commit that referenced this pull request Jan 27, 2013

@beberlei beberlei merged commit 0d7c5f0 into doctrine:master Jan 27, 2013

Owner

beberlei commented Jan 27, 2013

@SimonSimCity thank you for the pull request, very much appreciated!

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