-
Notifications
You must be signed in to change notification settings - Fork 231
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
add attribute driver support #692
Conversation
91b620a
to
9e56b2d
Compare
@IonBazan if you want to see whether static analysis is green or add some test feel free to require ODM's 2.3.x branch in composer.json. Later we'll drop the change once 2.3.0 is tagged properly |
Also it'd be great to check and update the docs. They definitely need some love since they still mention |
For the docs we could also add "PHP (attributes)" section for mapping examples: https://symfony.com/doc/current/bundles/DoctrineMongoDBBundle/first_steps.html#creating-a-document-class |
Alright, I've temporarily added |
As suggested in doctrine/mongodb-odm#2344 (comment), it would be nice to add some tests like in https://github.com/doctrine/DoctrineBundle/pull/1322/files |
9606285
to
5c76cd0
Compare
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.
LGTM, just some minor comments
@@ -176,7 +176,7 @@ The following configuration shows a bunch of mapping examples: | |||
mappings: | |||
MyBundle1: ~ | |||
MyBundle2: xml | |||
MyBundle3: { type: annotation, dir: Documents/ } | |||
MyBundle3: { type: attribute, dir: Documents/ } |
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 it would be better if we keep annotation
example and add a new one for attribute
#[MongoDB\Id] | ||
protected string $id; | ||
|
||
#[MongoDB\Field(type: 'string')] |
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.
Maybe in our examples we could suggest to use constants like Doctrine\ODM\MongoDB\Types\Type::STRING
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.
It should be changed in annotation examples then too but I think this is a candidate for a separate PR.
I don't see the constants usages in ORM docs neither and I don't think it's clear to new developers.
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.
After this one we can maybe think about #703 before any other new PR appears
This PR adds support for upcoming attribute mapping driver introduced in doctrine/mongodb-odm#2344.