-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Place a warning about the uses of traits in the documentation #10393
Conversation
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.
Nice job exploring this 👍
There is no real "official" statement whether or to which extent traits | ||
are supported in entity or mapped superclasses. | ||
|
||
Traits were added in PHP 5.4 more than 10 years ago, but at the same time |
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.
Time flies!
:doc:`overriding field association mappings in subclasses <tutorials/override-field-association-mappings-in-subclasses>`. Also, the | ||
tests in the ORM codebase (as of writing) cover traits only in two (!) places, | ||
namely the aforementioned override and in an edge-case for the deprecated | ||
"Entity Generator". |
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.
This paragraph is going to be hard to maintain: we will have to remember to remove this reference to "Entity Generator" when merging up to 3.0.x, and to change it any time we add any thing about traits in the docs. Maybe you should be more vague here?
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.
Changed that.
namely the aforementioned override and in an edge-case for the deprecated | ||
"Entity Generator". | ||
namely the aforementioned override and in an edge-case for a tool that will be | ||
removed in Doctrine 3.0. |
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 would replace the whole paragraph with "In fact, traits are rarely mentioned in the docs or in the tests. That way there is no divergence between branches.
There is no real "official" statement whether or to which extent traits | ||
are supported in entity or mapped superclasses. |
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.
But since this is the official documentation, we're about to make an official statement, aren't we? I find this paragraph irritating.
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.
You’re right.
So, what can we say?
“For the time being, traits are not officially supported.” ?
Updated text suggestions, please 👀 |
1329296
to
853e80c
Compare
Thanks @mpdude ! |
I am not sure if all this is 💯 correct, but I'd be happy to learn.