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

make Property an annotation, instead of adding new annotaiton Field #651

Closed
wants to merge 1 commit into from

Conversation

dbu
Copy link
Member

@dbu dbu commented Jul 15, 2015

see doctrine/phpcr-odm-documentation#74

we need no BC for @Field as it was never released.

while doing this, i noticed that in yml mapping, the list of properties is called fields which would be an argument in favour of using @Field after all. but @Property seems more natural for the annotations for me.

@dbu dbu force-pushed the property-vs-field branch 2 times, most recently from efff044 to 2fada9d Compare July 15, 2015 07:07
@@ -19,12 +19,8 @@

namespace Doctrine\ODM\PHPCR\Mapping\Annotations;
/**
* Base class for all the translatable properties (i.e. every property but Uuid and Version)
* @deprecated Use Property instead
*/
class TranslatableProperty extends Property
Copy link
Member Author

Choose a reason for hiding this comment

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

keeping for BC in case somebody extended it or has custom code that typechecks.

@lsmith77
Copy link
Member

Changelog also needs to be updated

@lsmith77
Copy link
Member

tests failing due to
PHP Fatal error: Class 'Doctrine\Tests\ODM\PHPCR\Mapping\Model\PropertyMappingObject' not found in /home/travis/build/doctrine/phpcr-odm/tests/Doctrine/Tests/ODM/PHPCR/Mapping/AbstractMappingDriverTest.php on line 22

@dbu
Copy link
Member Author

dbu commented Jul 15, 2015

ups. fixed, and tests where green. now updated the changelog.

@dantleech @wouterj @lsmith77 ok with going like this? if so, i will update the doc as well.

@lsmith77
Copy link
Member

+1 from me

@lsmith77 lsmith77 added review and removed wip/poc labels Jul 15, 2015
@wouterj
Copy link
Contributor

wouterj commented Jul 15, 2015

I'm -1 on this change, but as it's 3 vs 1, I think I have no option...

It's inconsistent with yaml, mongodb odm and couchdb odm. Also, it's a BC break for people having a custom annotation extending from Property (it know suddently becomes translatable, while the function of Property was to have something that's not translatable).

@dbu
Copy link
Member Author

dbu commented Jul 15, 2015

oh, so mongodb and couchdb use Field? that would indeed be a strong argument in favor of using Field here as well.

loosk like: http://doctrine-couchdb.readthedocs.org/en/latest/reference/basic-mapping.html#field-mapping

so i am 👎 on my own PR :-/

@lsmith77
Copy link
Member

alright .. then i will reactivate my docs PR to remove @Property

@lsmith77 lsmith77 closed this Jul 15, 2015
@lsmith77 lsmith77 removed the review label Jul 15, 2015
@lsmith77
Copy link
Member

@lsmith77 lsmith77 deleted the property-vs-field branch July 15, 2015 09:46
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.

None yet

3 participants