Skip to content

PHPCR-37 - Sync doctrine common metadata changes in master #85

Merged
merged 14 commits into from Dec 28, 2011

3 participants

@Ocramius
Doctrine member

Hi there!
This is the PR for task http://www.doctrine-project.org/jira/browse/PHPCR-37
It should also add requested feature http://www.doctrine-project.org/jira/browse/PHPCR-31
I am here just asking for an early review as I first have to fix newlines on some of the committed files (you will surely notice them) before eventually having this merged into master.

Tested on windows, php 5.3.8. Result is "Tests: 192, Assertions: 871, Incomplete: 1, Skipped: 2.". No unit tests have been added, which is also open for suggestions :)

As we're working on metadata drivers here, also wanted to know what to do with the Doctrine\ODM\PHPCR\Mapping\Driver\BuiltinDocumentsDriver, which I didn't fully understand.

Adding reference to @beberlei as he could need these changes too in other doctrine related projects.
Also wanted to know by anyone who eventually has information about that where and who is working on this in other doctrine projects (for reference and to help back).

Build Status (because of Midgard PHP 5.4 issues)

@lsmith77 lsmith77 commented on an outdated diff Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function getFqcnFromAlias($namespaceAlias, $simpleClassName)
+ {
+ return $this->dm->getConfiguration()->getDocumentNamespace($namespaceAlias)
+ . '\\' . $simpleClassName;
+ }
+
+ /**
+ * {@inheritdoc}
+ *
+ * @todo unclear usage of rootEntityFound
+ */
+ protected function doLoadMetadata($class, $parent, $rootEntityFound) {
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

opening bracket should be on the next line (same applies to the following methods)

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 Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
+ return new ClassMetadata($className);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function getFqcnFromAlias($namespaceAlias, $simpleClassName)
+ {
+ return $this->dm->getConfiguration()->getDocumentNamespace($namespaceAlias)
+ . '\\' . $simpleClassName;
+ }
+
+ /**
+ * {@inheritdoc}
+ *
+ * @todo unclear usage of rootEntityFound
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

@beberlei we were unsure what the purpose is here ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on the diff Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
@@ -1,278 +1,141 @@
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

as noted we need to fix the line endings in this file ..

@Ocramius
Doctrine member
Ocramius added a note Dec 26, 2011

Fixed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on the diff Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Mapping/MappingException.php
@@ -1,88 +1,88 @@
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

needs line ending fixing

@Ocramius
Doctrine member
Ocramius added a note Dec 26, 2011

Fixed :)

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 Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Mapping/MappingException.php
+
+ public static function classIsNotAValidDocument($className)
+ {
+ return new self('Class '.$className.' is not a valid document or mapped super class.');
+ }
+
+ public static function reflectionFailure($document, \ReflectionException $previousException)
+ {
+ return new self('An error occurred in ' . $document, 0, $previousException);
+ }
+
+ /**
+ * @param string $document The document's name
+ * @param string $fieldName The name of the field that was already declared
+ */
+ public static function duplicateFieldMapping($document, $fieldName) {
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

opening bracket is off ..

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 Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Mapping/MappingException.php
+ return new self('An error occurred in ' . $document, 0, $previousException);
+ }
+
+ /**
+ * @param string $document The document's name
+ * @param string $fieldName The name of the field that was already declared
+ */
+ public static function duplicateFieldMapping($document, $fieldName) {
+ return new self('Property "'.$fieldName.'" in "'.$document.'" was already declared, but it must be declared only once');
+ }
+
+ /**
+ * @param string $document The document's name
+ * @param string $fieldName The name of the field that was already declared
+ */
+ public static function missingTypeDefinition($document, $fieldName) {
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

opening bracket is off ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on the diff Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Proxy/ProxyFactory.php
@@ -1,339 +1,344 @@
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

line endings need fixing

@Ocramius
Doctrine member
Ocramius added a note Dec 26, 2011

Fixed :)

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 Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Configuration.php
+ *
+ * @param Cache $metadataCacheImpl
+ */
+ public function setMetadataCacheImpl(Cache $metadataCacheImpl)
+ {
+ $this->attributes['metadataCacheImpl'] = $metadataCacheImpl;
+ }
+
+ /**
+ * Gets the cache driver implementation that is used for the mapping metadata.
+ *
+ * @return Cache|null
+ */
+ public function getMetadataCacheImpl()
+ {
+ if($this->attributes['metadataCacheImpl'] === null) {
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

missing space after if

@Ocramius
Doctrine member
Ocramius added a note Dec 26, 2011

Removed (was code using during development)

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 Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
+
+ /**
+ * The used metadata driver.
+ *
+ * @var \Doctrine\ODM\PHPCR\Mapping\Driver\Driver
+ */
+ private $driver;
+
+ /**
+ * Creates a new factory instance that uses the given DocumentManager instance.
+ *
+ * @param $dm The DocumentManager instance
+ */
+ public function __construct(DocumentManager $dm)
+ {
+ $this->dm = $dm;
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

extra whitespace should be removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77
Doctrine member

We might be able to steal a few unit tests from CouchDB ODM ..

@dbu
Doctrine member
dbu commented Dec 26, 2011

thanks for this work ocramius!
about the tests: did you look at the code coverage report? would be great if we have the non-trivial methods covered by tests.

do we need to figure out what the rootEntityFound thing is or can we just merge? if all tests still run, and applications still work i'd say we could merge.

@sixty-nine when you work on the versioning stuff in january, you want to look at this pull request (if its not merged by then) when you do any new annotations...

@lsmith77
Doctrine member

@dbu: lets wait for some feedback from @beberlei before merging this. but its indeed quite exciting. especially since @Ocramius is "on the side" also implementing metadata caching.

@Ocramius
Doctrine member

@dbu: this PR is almost only code cleanup/removal, but I'm going to check code coverage asap :)

I guess we MUST first know what rootEntityFound should look like... Can't just implement a feature because "it works! don't touch it!" :P

@lsmith77: if this stuff is not yet landed on other doctrine/* projects I'll first write this stuff for the others before getting back here (and I'm also curious about the status of OXM, which is sadly a bit stuck :( )

@Ocramius
Doctrine member

I've just uploaded the test coverage results to http://marco-pivetta.com/phpcr-odm-sync-doctrine-common-metadata-changes/coverage/
Doesn't seem that terrible, but still, we actually could need to borrow some tests...

@lsmith77 lsmith77 commented on the diff Dec 26, 2011
lib/Doctrine/ODM/PHPCR/Configuration.php
@@ -19,6 +19,7 @@
namespace Doctrine\ODM\PHPCR;
+use Doctrine\Common\Cache\Cache;
@lsmith77
Doctrine member
lsmith77 added a note Dec 26, 2011

this can probably be removed?

@Ocramius
Doctrine member
Ocramius added a note Dec 26, 2011

I use it for setMetadataCacheImpl(Cache $metadataCacheImpl) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77
Doctrine member

can you enable travis-ci for this fork? at any rate .. looks good to me .. lets hop that @beberlei shows his face soon to review :)

@lsmith77 lsmith77 commented on an outdated diff Dec 27, 2011
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
{
- if (isset($this->loadedAliases[$alias])) {
- return $this->loadedAliases[$alias];
+ if ($metadata = parent::getMetadataFor($className)) {
@lsmith77
Doctrine member
lsmith77 added a note Dec 27, 2011

i think its better to do the assignment outside of the if statement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius
Doctrine member

As explained by @beberlei, the doLoadMetadata method should be fine for now, but will require some editing if PHPCR-ODM starts supporting inheritance.
Build looks ok to me: http://travis-ci.org/Ocramius/phpcr-odm/builds/454023

@lsmith77 lsmith77 merged commit 5842eb3 into doctrine:master Dec 28, 2011
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.