Added MongoDB support #1

smilesrg · 2015-06-19T08:54:15Z

Added MongoDB support

sagikazarmark · 2015-06-19T10:21:49Z

composer.json

@@ -22,10 +22,14 @@
        "source": "https://github.com/portphp/doctrine",
        "docs": "https://portphp.readthedocs.org"
    },
+    "minimum-stability": "dev",    


I think this kind of configuration should go to the bottom of the file: most people are rather interested in requirements than stability settings for development.

sagikazarmark · 2015-06-19T10:32:40Z

@smilesrg Awesome work. 👍

Added a few comments though.

@ddeboer @Baachi thoughts?

ddeboer · 2015-06-19T13:39:25Z

composer.json

-        "doctrine/orm": "~2.4"
+        "doctrine/orm": "~2.4",
+        "doctrine/mongodb": "dev-master",
+        "doctrine/mongodb-odm": "dev-master"


Why these aggressive dependencies? @sagikazarmark And I think we should create a separate doctrine-mongo-adapter and move this there.

I thought the plan was to support object manager in one adapter.

Yeah, we decided that in portphp/portphp#13. However, that means the dependencies here need to be moved to require-dev.

Ah, you already mentioned that above.

Then why do we need a separate mongo adapter?

You’re right, we don’t (as long as we move the deps to require-dev).

Conflicts: src/DoctrineWriter.php

smilesrg · 2015-07-16T10:13:38Z

@sagikazarmark @ddeboer Please accept portphp/portphp#26

sagikazarmark · 2015-07-16T10:44:36Z

See my comment on that PR

smilesrg · 2015-07-16T14:10:06Z

@sagikazarmark https://travis-ci.org/portphp/doctrine-adapter/jobs/71258849 fails again on php5.4 ... why?

sagikazarmark · 2015-07-16T15:42:29Z

Not on PHP 5.4, but with lowest dependencies. One of the dependencies version must be higher.

You can try it:

$ composer update --prefer-lowest --prefer-stable

and run tests. Try moving the lowest dependency up.

smilesrg · 2015-07-18T16:14:56Z

@sagikazarmark Done! :-)

sagikazarmark · 2015-07-19T13:42:53Z

Great. Awaiting for response from @ddeboer

This reverts commit 942c01c.

Baachi · 2015-07-20T20:39:06Z

src/DoctrineReader.php

-        $query = $this->objectManager->createQuery(
-            sprintf('SELECT COUNT(o) FROM %s o', $this->objectName)
-        );
+        $query = $this->objectManager->createQueryBuilder()


Why do you use the query builder here?

For getting this method work for both Doctrine ORM and Doctrine MongoDB ODM.

Baachi · 2015-07-20T20:42:07Z

src/DoctrineWriter.php

@@ -262,47 +287,50 @@ protected function disableLogging()
     */
    protected function reEnableLogging()
    {
-        $config = $this->entityManager->getConnection()->getConfiguration();
+        //TODO: do we need to add support for MongoDB logging?
+        if (!($this->objectManager instanceof \Doctrine\ORM\EntityManager)) return;


Should throw an exception if the odm is used.

Why? If ODM is used, it will just do nothing. This is expected behaviour. Logger should be disabled/enabled only for EntityManager

Yeah i think this isn't expected. If i call an method and he does not throw any exception, i would expect that he works.

What's your proposition then? If the ODM is used, this method shouldn't be called. Should I change the code this way?

I would simply throw an Exception if the objectManager property isn't a Doctrine\ORM\EntityManager.

Then this exception should be handled in the client code.

Exactly. If the user tries some illegal actions, an exception should be thrown.

Alternatively we could create a OdmReader and an OrmReader which extends a common class, so that specific features are only present when they are necessary. @smilesrg @Baachi what do you think?

It's not a user tries to do illegal actions. The logger for SQL is disabled to speed up the write operations. Honesty, I don't know how to disable the logging for MongoDB. That's why this functionality is supported for Doctrine ORM only.

Ah, so this is an internal thing. Still, a separated class would simply solve this issue.

Baachi · 2015-07-20T21:18:28Z

Hey Sergey,

i would really say thank you for your work. Really great PR 👍

I'm 👍 on this.

smilesrg · 2015-07-20T21:21:11Z

Thanks, Baachi :-)

sagikazarmark · 2015-07-26T12:12:49Z

I agree with @Baachi 👍

ddeboer · 2015-08-02T12:36:33Z

@smilesrg Looks great! can you please squash commits?

smilesrg · 2015-08-03T09:05:30Z

@ddeboer done

smilesrg · 2015-08-10T09:09:08Z

@ddeboer any news on acception this PR?

smilesrg · 2017-01-27T11:56:26Z

@ddeboer wow, after 1.5 years it has been merged today :-) Hope my contribution will be helpful :-)

ddeboer · 2017-01-27T11:57:19Z

Yeah, sorry about that delay. It’s definitely helpful! Thanks for your work. 😄

Added MongoDB support

9cd294e

smilesrg mentioned this pull request Jun 19, 2015

Added MongoDB support. ddeboer/data-import#83

Closed

Added mongo extension to travis

5f560d5

sagikazarmark reviewed Jun 19, 2015
View reviewed changes

ddeboer reviewed Jun 19, 2015
View reviewed changes

smilesrg added 3 commits July 16, 2015 12:54

Merge branch 'master' of https://github.com/portphp/doctrine-adapter

961ea74

Conflicts: src/DoctrineWriter.php

Updated composer.json

65c7223

Fixes.

6eb1f45

smilesrg added 3 commits July 16, 2015 15:50

Added UnsupportedDatabaseTypeException

528bb6c

Fixed namespaces paths

fcb1025

Fixed unit tests

523a7c4

phpunit version update

270ebdf

smilesrg added 2 commits July 20, 2015 21:37

Corrected namespace import.

942c01c

Revert "Corrected namespace import."

a19b1f7

This reverts commit 942c01c.

Baachi reviewed Jul 20, 2015
View reviewed changes

sagikazarmark mentioned this pull request Jul 26, 2015

add optional QueryBuilder #3

Closed

smilesrg added 2 commits August 3, 2015 11:56

Added MongoDB support

5a31a2b

Merge branch 'master' of https://github.com/smilesrg/doctrine-adapter

05fffbe

ddeboer mentioned this pull request Jan 27, 2017

Add MongoDB support #4

Closed

ddeboer merged commit db7ab31 into portphp:master Jan 27, 2017

Added MongoDB support #1

Added MongoDB support #1

Conversation

smilesrg commented Jun 19, 2015

Choose a reason for hiding this comment

sagikazarmark commented Jun 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smilesrg commented Jul 16, 2015

sagikazarmark commented Jul 16, 2015

smilesrg commented Jul 16, 2015

sagikazarmark commented Jul 16, 2015

smilesrg commented Jul 18, 2015

sagikazarmark commented Jul 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baachi commented Jul 20, 2015

smilesrg commented Jul 20, 2015

sagikazarmark commented Jul 26, 2015

ddeboer commented Aug 2, 2015

smilesrg commented Aug 3, 2015

smilesrg commented Aug 10, 2015

smilesrg commented Jan 27, 2017

ddeboer commented Jan 27, 2017 • edited

ddeboer commented Jan 27, 2017 •

edited