-
Notifications
You must be signed in to change notification settings - Fork 450
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
Clean integration of CouchDB #524
base: master
Are you sure you want to change the base?
Conversation
@gaubej note you can use |
@dkarlovi thanks but I can't get this Makefile work. I must rebuild an image with php-mongo extension to be able to install dependencies before launch the ci. |
…ng couchdb in dependencies.
I have updated all necessary for the CI, but I have problem with method to delete expired stuff with couchdb. I am new in Symfony so if someone can help me or merge so I can move forward. Thanks. |
composer.json
Outdated
@@ -20,7 +20,8 @@ | |||
"friendsofsymfony/oauth2-php": "~1.1", | |||
"symfony/framework-bundle": "~3.0|^4.0", | |||
"symfony/security-bundle": "~3.0|^4.0", | |||
"symfony/dependency-injection": "^2.8|~3.0|^4.0" | |||
"symfony/dependency-injection": "^2.8|~3.0|^4.0", | |||
"alcaeus/mongo-php-adapter": "^1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to Mongo ODM, don't need it.
@@ -34,6 +35,9 @@ | |||
"doctrine/mongodb-odm": "~1.0", | |||
"doctrine/doctrine-bundle": "~1.0", | |||
"doctrine/orm": "~2.2", | |||
"doctrine/couchdb": "1.0.x-dev", | |||
"doctrine/couchdb-odm": "dev-master", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH this is the problem I'm seeing here, I'd like for them to do a release before we integrate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree, CouchDB 2.1 was just release so I hope some people can update this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, nobody is working on couchdb-odm at the moment. I wouldn't count on a stable release anytime soon. With that in mind, I'm not sure if we should add integration for CouchDB at this time.
composer.json
Outdated
}, | ||
"config": { | ||
"platform": { | ||
"ext-mongo": "1.6.16" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for Mongo ODM, remove.
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace FOS\OAuthServerBundle\CouchDocument; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move MongoDB to make way for CouchDB? This way, one of them seems "more important".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Apache CouchDB can have real advantage. For my project, I use it for scalability, and master-master replication capability. But, like you said, library are not up-to-date, I try to make some PR to keep these libraries usable with Symfony 3.X but they are not ready for 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, should we move the MongoDB models to say FOS\Document\MongoDB
to make room for other ODM implementations?
CouchDocument/AuthCodeManager.php
Outdated
; | ||
|
||
return $result['n']; | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't the existing code work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CouchDB work with JS view to make request, I don't have function to request and check date, I have to find a way to make this view and import this view in CouchDB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but ODM abstracts all of it away. My questions was: why can't existing code (or something very similar to it) do the same task.
use Symfony\Component\Form\Form; | ||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpFoundation\RequestStack; | ||
use Symfony\Component\HttpFoundation\Response; | ||
use Symfony\Component\HttpFoundation\Session\SessionInterface; | ||
use Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, you don't typehint against a debug version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally don't understand why the initial version is not working and your comment. I just change this following error message, but I'm really not an expert.
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace FOS\OAuthServerBundle\CouchDocument; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, should we move the MongoDB models to say FOS\Document\MongoDB
to make room for other ODM implementations?
@@ -19,8 +19,7 @@ | |||
"php": "^7.1", | |||
"friendsofsymfony/oauth2-php": "~1.1", | |||
"symfony/framework-bundle": "~3.0|^4.0", | |||
"symfony/security-bundle": "~3.0|^4.0", | |||
"symfony/dependency-injection": "^2.8|~3.0|^4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removing this?
@@ -78,6 +78,20 @@ public function load(array $configs, ContainerBuilder $container) | |||
} | |||
} | |||
|
|||
// Handle the MongoDB document manager name in a specific way as it does not have a registry to make it easy | |||
// TODO: change it when bumping the requirement to Symfony 2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these requirements are already bumped. If this PR is to be merged to master
, it should work with Symfony 3.0 onwards.
Hi,
I have added model for couchdb, with indexation to be able to authentify users.
I don't know if all is optimize and clean, but that's OK with my application.
Regards,