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

[WIP] Phpcr support #2162

Closed
wants to merge 35 commits into from

Conversation

ElectricMaxxx
Copy link

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
License MIT

This one is really hot-WIP. At the end i wanna get PHPCR support at the API together with mongoDB. That is the reason why i depend on @alanpoulain's branch. We also have to be carefull to devide common stuff and implementation stuff.

Issues i faced and goanna face:

  • Child Assosiation are different to Reference asscociations in a phpcr
  • we do not have any mappedBy|inversedBy
  • Creation and Movement of Nodes in the tree can be restricted.

@dbu please feel the ping to be informed, no hurry to invest time,but be informed :-)

Sam Van der Borght and others added 30 commits November 8, 2016 11:41
- add symfony mongo test
- add mongo on travis & appveyor
- add symfony mongo test
- add mongo on travis & appveyor
 into samvdb-odm-support

# Conflicts:
#	appveyor.yml
#	composer.json
#	src/Bridge/Doctrine/Filter/AbstractFilter.php
#	src/Bridge/Symfony/Bundle/DependencyInjection/Compiler/DoctrineQueryExtensionPass.php
#	src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/Compiler/DoctrineQueryExtensionPassTest.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php
 into samvdb-odm-support

# Conflicts:
#	appveyor.yml
#	composer.json
#	src/Bridge/Doctrine/Filter/AbstractFilter.php
#	src/Bridge/Symfony/Bundle/DependencyInjection/Compiler/DoctrineQueryExtensionPass.php
#	src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/Compiler/DoctrineQueryExtensionPassTest.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php
Copy link

@dbu dbu left a comment

Choose a reason for hiding this comment

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

interesting! i would love to see api platform support phpcr and mongodb.

is it possible that this PR started from some branch or something? there are a lot of commits in the PR, and a lot of changes that seem unrelated.

@@ -16,6 +16,7 @@
"php": ">=7.1",

"doctrine/inflector": "^1.0",
"jackalope/jackalope-doctrine-dbal": "^1.3",
Copy link

Choose a reason for hiding this comment

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

does api platform not work without phpcr? i fear that this is an "optional dependency". would it be possible to do a api-platform/phpcr integration package instead?

Copy link
Author

Choose a reason for hiding this comment

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

mh maybe at the end. but i do not know if we want to move this bundle in the same virtual packagage situation, we are, shall we?

@@ -28,6 +29,7 @@
"require-dev": {
"symfony/http-foundation": "^3.1@dev || ^4.0@dev",

"alcaeus/mongo-php-adapter": "^1.1",
Copy link

Choose a reason for hiding this comment

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

i would recommend to not mix mongodb and phpcr support into one pull request. it will be easier to review if its first one and then the other once that goes through.

@@ -113,9 +122,10 @@ public function removeAcceptHeaderBeforeScenario()
*/
public function createDatabase()
{
$this->schemaTool->dropSchema($this->classes);
$this->isOrm() && $this->schemaTool->dropSchema($this->classes);
$this->isOdm() && $this->schemaManager->dropDatabases();
Copy link

Choose a reason for hiding this comment

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

could it make sense to split the context into two subclasses instead?

Copy link
Author

Choose a reason for hiding this comment

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

There is a lot to separate at the end. I think we should move the complete extension stuff into separate classes as we did it in our sonata-admin-integration-bundle when the branches a really separated, we should see what each implementation is needing.

@ElectricMaxxx
Copy link
Author

@dbu is tried to do it on top of the other branch, but selected the wrong one. Now i can not change it anymore.

@ElectricMaxxx
Copy link
Author

will move that one to the right branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants