Skip to content

cleanup xml mapping names #279

Merged
merged 6 commits into from May 23, 2013

6 participants

@dbu
Doctrine member
dbu commented Apr 19, 2013

FINAL:

  • the xml attribute for the class property is now field anywhere, never name.
  • all xml names where converted to lower-case with - separator.
  • the annotation option to specify the phpcr property is now called property and no longer name.

this is quite a BC break when using xml mappings.

@dbu dbu and 2 others commented on an outdated diff Apr 19, 2013
lib/Doctrine/ODM/PHPCR/Mapping/Driver/XmlDriver.php
@@ -107,6 +108,13 @@ public function loadMetadataForClass($className, ClassMetadata $class)
$mapping[$key] = ('true' === $mapping[$key]) ? true : false;
}
}
+ $mapping['fieldName'] = $mapping['name'];
+ if (isset($mapping['property'])) {
+ $mapping['name'] = $mapping['property'];
+ unset($mapping['property']);
+ } else {
+ unset($mapping['name']);
+ }
@dbu
Doctrine member
dbu added a note Apr 19, 2013

this is pretty stupid. in the ClassMetadata we use name to denote the phpcr property. in xml i now made it clean, but this leads to this very weird juggling around fields.

should we rename the thing that tells the phpcr property name (defaults to the field name) to property? would be much better, but would be a BC break for the annotations as well.

@dantleech
dantleech added a note Apr 29, 2013

+1 for making things cleaner :) Reading that code makes my head spin..

@lsmith77
Doctrine member
lsmith77 added a note May 3, 2013

+1

@dbu
Doctrine member
dbu added a note May 5, 2013

will do once we have agreement on the complete xml schema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu and 2 others commented on an outdated diff Apr 19, 2013
lib/Doctrine/ODM/PHPCR/Mapping/Driver/XmlDriver.php
}
+ $mixins[] = (string)$attributes['type'];
@dbu
Doctrine member
dbu added a note Apr 19, 2013

@uwej711 i renamed the xml attribute for a mixin from name to type. more telling, do you agree?

@uwej711
uwej711 added a note Apr 19, 2013
@dantleech
dantleech added a note Apr 29, 2013

I think there should be a space between the type and the variable. . (string) $attributes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Apr 19, 2013
lib/Doctrine/ODM/PHPCR/Mapping/Driver/XmlDriver.php
@@ -84,14 +84,15 @@ public function loadMetadataForClass($className, ClassMetadata $class)
$mixins = array();
foreach ($xmlRoot->mixin as $mixin) {
$attributes = $mixin->attributes();
- if (isset($attributes['name'])) {
- $mixins[] = (string)$attributes['name'];
+ if (! isset($attributes['type'])) {
+ throw new MappingException('<mixin> missing mandatory type attribute');
@dbu
Doctrine member
dbu added a note Apr 19, 2013

i am not sure about validation. eventually we probably should have an xml schema for this. some places do validate, others seem to do too careful isset things that end up in invalid metadata. it will be detected later but with less understandable errors than if we would report that here.

@dbu
Doctrine member
dbu added a note May 5, 2013

now that we do have the schema, can we trust things are validated with the schema?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu referenced this pull request in symfony-cmf/block-bundle Apr 23, 2013
Closed

Clean up code and remove duplication in BlockBundle #52

3 of 3 tasks complete
@dbu
Doctrine member
dbu commented Apr 25, 2013

this PR tries to follow the orm mapping logic. one thing that was kind of confusing to me was that the <id and <field mapping use an xml attribute name= to say what entity field this maps to, while relations use the field= attribute. is that a decision with reasons, or it just happens, or even some BC legacy that we rather should not copy? i translated that for phpcr to use "field" for all relations (reference, referrer, child, parent) and "name" for everything else (actual properties, id, nodename, node type, ...)

/cc @beberlei @ocramius

@Ocramius
Doctrine member

@dbu does PHPCR-ODM have an XSD?

@dbu
Doctrine member
dbu commented Apr 25, 2013

unfortunately not yet. i know its not the same, but this PR updates quite some xml mappings that show many of the cases, maybe that can help to see how it would look.
i guess there is nothing easier than building that xsd by hand, is there?

@Ocramius
Doctrine member

@dbu I'm not aware of any tools for that, but XML mappings without an XSD is a real pain - you should probably fix that first since it gives you an overhead of what you allow and what not in a single document :)

@dbu
Doctrine member
dbu commented Apr 29, 2013

i started the schema by copying the orm one and adapting. only came to the middle for now but wanted to push before somebody else does the same. i added quite some TODO comments. maybe @lsmith77 has some input there?

@dbu dbu referenced this pull request in symfony-cmf/symfony-cmf May 1, 2013
Closed

Ensure all bundles use XML mapping #159

@lsmith77 lsmith77 commented on an outdated diff May 3, 2013
doctrine-phpcr-odm-mapping.xsd
+ </xs:element>
+
+ <xs:complexType name="emptyType">
+ <xs:sequence>
+ <xs:any minOccurs="0" maxOccurs="unbounded" namespace="##other"/>
+ </xs:sequence>
+ <xs:anyAttribute namespace="##other"/>
+ </xs:complexType>
+
+ <xs:complexType name="cascade-type">
+ <xs:sequence>
+ <xs:element name="cascade-all" type="phpcr:emptyType" minOccurs="0"/>
+ <xs:element name="cascade-persist" type="phpcr:emptyType" minOccurs="0"/>
+ <xs:element name="cascade-merge" type="phpcr:emptyType" minOccurs="0"/>
+ <xs:element name="cascade-remove" type="phpcr:emptyType" minOccurs="0"/>
+ <!-- TODO: orm has no detach -->
@lsmith77
Doctrine member
lsmith77 added a note May 3, 2013

i guess that is am omission there .. @Ocramius ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff May 3, 2013
doctrine-phpcr-odm-mapping.xsd
+ </xs:complexType>
+
+ <xs:simpleType name="lifecycle-callback-type">
+ <xs:restriction base="xs:token">
+ <xs:enumeration value="prePersist"/>
+ <xs:enumeration value="postPersist"/>
+ <xs:enumeration value="preUpdate"/>
+ <xs:enumeration value="postUpdate"/>
+ <xs:enumeration value="preRemove"/>
+ <xs:enumeration value="postRemove"/>
+ <xs:enumeration value="postLoad"/>
+ <xs:enumeration value="preFlush"/>
+ <!-- move is phpcr specific -->
+ <xs:enumeration value="preMove"/>
+ <xs:enumeration value="postMove"/>
+ <!-- TODO orm has no onFlush, postFlush, onClear, loadClassMetadata -->
@lsmith77
Doctrine member
lsmith77 added a note May 3, 2013

i guess that is am omission there .. @Ocramius ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 and 1 other commented on an outdated diff May 3, 2013
doctrine-phpcr-odm-mapping.xsd
+ <xs:any minOccurs="0" maxOccurs="unbounded" namespace="##other"/>
+ </xs:sequence>
+ <xs:attribute name="type" type="phpcr:lifecycle-callback-type" use="required" />
+ <xs:attribute name="method" type="xs:NMTOKEN" use="required" />
+ <xs:anyAttribute namespace="##other"/>
+ </xs:complexType>
+
+ <xs:complexType name="lifecycle-callbacks">
+ <xs:sequence>
+ <xs:element name="lifecycle-callback" type="phpcr:lifecycle-callback" minOccurs="1" maxOccurs="unbounded" />
+ <xs:any minOccurs="0" maxOccurs="unbounded" namespace="##other"/>
+ </xs:sequence>
+ <xs:anyAttribute namespace="##other"/>
+ </xs:complexType>
+
+ <!-- TODO: what is entity-listener? -->
@lsmith77
Doctrine member
lsmith77 added a note May 3, 2013

we do not have this yet in the ODM, its new in ORM 2.4
http://docs.doctrine-project.org/en/latest/reference/events.html#entity-listeners

@dbu
Doctrine member
dbu added a note May 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff May 3, 2013
doctrine-phpcr-odm-mapping.xsd
+ <xs:enumeration value="false"/>
+ <xs:enumeration value="SIMPLE"/>
+ <xs:enumeration value="FULL"/>
+ </xs:restriction>
+ </xs:simpleType>
+
+ <xs:simpleType name="generator-strategy">
+ <xs:restriction base="xs:token">
+ <xs:enumeration value="REPOSITORY"/>
+ <xs:enumeration value="ASSIGNED"/>
+ <xs:enumeration value="PARENT"/>
+ <!-- custom not yet? -->
+ </xs:restriction>
+ </xs:simpleType>
+
+ <!-- TODO -->
@lsmith77
Doctrine member
lsmith77 added a note May 3, 2013

can be removed i guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 and 1 other commented on an outdated diff May 3, 2013
doctrine-phpcr-odm-mapping.xsd
+ <xs:enumeration value="ASSIGNED"/>
+ <xs:enumeration value="PARENT"/>
+ <!-- custom not yet? -->
+ </xs:restriction>
+ </xs:simpleType>
+
+ <!-- TODO -->
+ <xs:simpleType name="fk-action">
+ <xs:restriction base="xs:token">
+ <xs:enumeration value="CASCADE"/>
+ <xs:enumeration value="RESTRICT"/>
+ <xs:enumeration value="SET NULL"/>
+ </xs:restriction>
+ </xs:simpleType>
+
+ <!-- TODO -->
@lsmith77
Doctrine member
lsmith77 added a note May 3, 2013

can be removed i guess

@dbu
Doctrine member
dbu added a note May 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff May 3, 2013
doctrine-phpcr-odm-mapping.xsd
+ <xs:simpleType name="fetch-type">
+ <xs:restriction base="xs:token">
+ <xs:enumeration value="EAGER"/>
+ <xs:enumeration value="LAZY"/>
+ <xs:enumeration value="EXTRA_LAZY"/>
+ </xs:restriction>
+ </xs:simpleType>
+
+ <xs:complexType name="field">
+ <xs:sequence>
+ <xs:element name="options" type="phpcr:options" minOccurs="0" />
+ <xs:any minOccurs="0" maxOccurs="unbounded" namespace="##other"/>
+ </xs:sequence>
+ <xs:attribute name="name" type="xs:NMTOKEN" use="required" />
+ <xs:attribute name="type" type="xs:NMTOKEN" default="string" />
+ <!-- TODO: id bool really? -->
@lsmith77
Doctrine member
lsmith77 added a note May 3, 2013

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff May 3, 2013
doctrine-phpcr-odm-mapping.xsd
+ <xs:attribute name="nullable" type="xs:boolean" default="false" />
+ <xs:anyAttribute namespace="##other"/>
+ </xs:complexType>
+
+ <xs:complexType name="generator">
+ <xs:sequence>
+ <xs:any minOccurs="0" maxOccurs="unbounded" namespace="##other"/>
+ </xs:sequence>
+ <xs:attribute name="strategy" type="phpcr:generator-strategy" use="optional" default="AUTO" />
+ <xs:anyAttribute namespace="##other"/>
+ </xs:complexType>
+
+ <xs:complexType name="id">
+ <xs:sequence>
+ <xs:element name="generator" type="phpcr:generator" minOccurs="0" />
+ <!-- TODO: do we need this custom-id-generator? -->
@lsmith77
Doctrine member
lsmith77 added a note May 3, 2013

i dont think that we support this yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff May 3, 2013
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -789,6 +789,8 @@ protected function validateAndCompleteAssociationMapping($mapping, ClassMetadata
}
$mapping['sourceDocument'] = $this->name;
+ // TODO: i think its a bug to not have targetDocument here.
+ // we run into exceptions if its not set: undefined index on line 1117
@lsmith77
Doctrine member
lsmith77 added a note May 3, 2013

we do not require a targetDocument so the issue will need to be addressed in getAssociationTargetClass()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff May 5, 2013
doctrine-phpcr-odm-mapping.xsd
+ </xs:element>
+
+ <xs:complexType name="emptyType">
+ <xs:sequence>
+ <xs:any minOccurs="0" maxOccurs="unbounded" namespace="##other"/>
+ </xs:sequence>
+ <xs:anyAttribute namespace="##other"/>
+ </xs:complexType>
+
+ <xs:complexType name="cascade-type">
+ <xs:sequence>
+ <xs:element name="cascade-all" type="phpcr:emptyType" minOccurs="0"/>
+ <xs:element name="cascade-persist" type="phpcr:emptyType" minOccurs="0"/>
+ <xs:element name="cascade-merge" type="phpcr:emptyType" minOccurs="0"/>
+ <xs:element name="cascade-remove" type="phpcr:emptyType" minOccurs="0"/>
+ <!-- TODO: orm has no detach -->
@dbu
Doctrine member
dbu added a note May 5, 2013

@Ocramius is that an omission in the orm xml schema?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff May 5, 2013
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -1114,6 +1114,7 @@ public function getAssociationTargetClass($fieldName)
throw new InvalidArgumentException("Association name expected, '$fieldName' is not an association in '{$this->name}'.");
}
+ // TODO this may be not set. what happens if we would return null then?
@dbu
Doctrine member
dbu added a note May 5, 2013

shall i create another blocker in the phpcr-odm jira? i don't want to fix this in this PR but get things merged...

@lsmith77
Doctrine member
lsmith77 added a note May 5, 2013

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff May 5, 2013
...ts.ODM.PHPCR.Mapping.Model.ChildMappingObject.dcm.xml
@@ -1,8 +1,11 @@
<?xml version="1.0" encoding="UTF-8"?>
-<doctrine-mapping>
+<doctrine-mapping xmlns="http://doctrine-project.org/schemas/phpcr-odm/phpcr-mapping"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://doctrine-project.org/schemas/phpcr-odm/phpcr-mapping
+ http://www.doctrine-project.org/schemas/phpcr-odm/doctrine-phpcr-odm-mapping.xsd">
@dbu
Doctrine member
dbu added a note May 5, 2013

@Ocramius, @beberlei is this the correct path for the schema? or what else? how to get the schema to the server in the right location?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu
Doctrine member
dbu commented May 5, 2013

ok the xml schema is created and added. now the question remaining:

  • schema location?
  • when to call the class field "field", when to call it "name"?
  • should we rename the properties annotation attribute name to property for consistence?
  • the annotations have lots of custom annotations for properties: one for uuid, one for each property type (string, binary, long, and so on). should we have the same for xml? or at least uuid as special case?
@dbu
Doctrine member
dbu commented May 13, 2013

another round of cleanups, now we call the model property "name" for all xml mappings, the annotation attribute "name" is now "property" except for the child annotation where it is "nodeName". remaining open:

  • schema location and inconsistencies with orm schema
  • the annotations have lots of custom annotations for properties: one for uuid, one for each property type (string, binary, long, and so on). should we have the same for xml? or at least uuid as special case?
@beberlei
Doctrine member

@dbu schema location should be on github, then you don't need to do deploy it somewhere.

For the naming that is entirely up to you.

@dbu
Doctrine member
dbu commented May 23, 2013

good idea with the github schema location, thanks.

i was wondering if there is some philosophy behind the attribute names and if we can avoid some legacy issue the orm has. we opted for using "name" everywhere now.

@dbu dbu merged commit 27d27df into master May 23, 2013

1 check passed

Details default The Travis CI build passed
@dbu dbu deleted the cleanup-xml-mappings branch May 23, 2013
@dbu
Doctrine member
dbu commented May 23, 2013

i created http://www.doctrine-project.org/jira/browse/DDC-2468 for the orm schema surprises i had.

@lsmith77 lsmith77 referenced this pull request in symfony-cmf/block-bundle May 25, 2013
Closed

Annotation Exception #62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.