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

Clean integration of CouchDB #524

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

gaubej
Copy link

@gaubej gaubej commented Jan 25, 2018

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,

@dkarlovi
Copy link
Contributor

@gaubej note you can use make before committing to fix CS and run checks before committing / pushing.

@gaubej
Copy link
Author

gaubej commented Jan 30, 2018

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

@gaubej
Copy link
Author

gaubej commented Jan 30, 2018

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"
Copy link
Contributor

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",
Copy link
Contributor

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.

Copy link
Author

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

Copy link

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"
Copy link
Contributor

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;
Copy link
Contributor

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

Copy link
Author

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

Copy link
Contributor

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?

;

return $result['n'];
return null;
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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;
Copy link
Contributor

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.

Copy link
Author

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;
Copy link
Contributor

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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing this?

@dkarlovi dkarlovi self-assigned this Feb 14, 2018
@@ -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
Copy link
Contributor

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.

@dkarlovi dkarlovi removed their assignment Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants