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

Improve setup docs regarding Doctrine ODM #483

Conversation

winterstefan
Copy link

Goal:

  • Improve setup documentation
  • Use equal configuration for ODM as for the ORM

Steps:

  • Add user field where necessary (ORM already has them).
  • Use Doctrine\ODM\MongoDB\Mapping\Annotations configuration (same as for ORM).
  • Remove getClient() and setClient() since the super class already implements getter / setter.

Copy link

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

👍

@dinamic
Copy link
Contributor

dinamic commented Jan 12, 2018

Some prefer XML, other prefer YML. You seem to prefer annotations.

IMO would be best to have as much docs and examples as possible.

P.S Merging master to your branch would make the Travis CI build green.

@alcaeus
Copy link

alcaeus commented Jan 12, 2018

@dinamic See PR description:

Use Doctrine\ODM\MongoDB\Mapping\Annotations configuration (same as for ORM).

As for Doctrine recommendations, we recommend using XML for libraries or when you want to decouple your domain logic from your mapping. In other cases, use annotations.

@dkarlovi
Copy link
Contributor

when you want to decouple your domain logic from your mapping. In other cases,

There shouldn't be "other cases", when do you NOT want to do this?

@alcaeus
Copy link

alcaeus commented Jan 12, 2018

One could argue that even using XML ties your domain logic to your mapping, but that's outside the scope here. I was merely stating the reason for the change.

@winterstefan winterstefan force-pushed the improve-setup-documentation branch from 869f05a to 5989d0d Compare January 12, 2018 13:21
@dkarlovi
Copy link
Contributor

Well, you obviously need the mapping somewhere, but IMO promoting annotations has done some harm here, it's now seen as a go-to way of doing things even though it shouldn't be.

@alcaeus
Copy link

alcaeus commented Jan 12, 2018

but IMO promoting annotations has done some harm here

In that case, feel free to change the ORM documentation. The primary goal was to have consistent documentation and ensure the ODM documentation produces a working result.

@dkarlovi
Copy link
Contributor

Sorry, didn't mean (just) here, I mean overall in the community.

Annotations should be a quick way to do something short-term, but in the long run you'll end up wanting to have used something else. :)

@alcaeus
Copy link

alcaeus commented Jan 12, 2018

That's subjective - while not optimal I believe annotations provide a nice balance between speed and "clean code feel".

Returning to the topic, what would you suggest for the issue at hand? ODM docs are actually broken and should be fixed. Would you rather drop XML for now and reintroduce it in a docs expansion later or would you rather change ORM docs first?

@dkarlovi
Copy link
Contributor

That's subjective

Exactly why we should follow the established way of having all three in the docs. Is there any way we could do that?

@alcaeus
Copy link

alcaeus commented Jan 12, 2018

all three

Please let's leave out YAML. We're dropping support for YAML in ODM 2.0 and ORM 3.0.

@dkarlovi
Copy link
Contributor

Please let's leave out YAML.

That's 1/3 of work already done! 😆 So we need XML for ORM and annotations for ODM? Seems doable, do you agree?

@alcaeus
Copy link

alcaeus commented Jan 12, 2018

Seems doable, do you agree?

Absolutely. @winterstefan can you make the changes for ODM? I can make the changes for ORM unless you prefer to do them as well 😉

@alcaeus
Copy link

alcaeus commented Jul 9, 2019

@winterstefan still up to work on this PR or would you like someone else to finish up? Note that #558 brings in annotations for ODM, so we could take that part of the docs from there.

@winterstefan
Copy link
Author

@alcaeus Sorry, my bad. Actually I'm pretty out of this topic. Would be cool for someone else to finish up.

@alcaeus
Copy link

alcaeus commented Jul 11, 2019

Fair enough, I know just the guy to do it 😉

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.

4 participants