[WIP] Abstract Mapping namespace into common code #71

Merged
merged 9 commits into from Nov 28, 2011

Conversation

Projects
None yet
6 participants
Owner

beberlei commented Oct 26, 2011

Abstract Mapping namespace into common code that can be shared across all implementations

Owner

beberlei commented Oct 26, 2011

This still needs to move over the tests.

+ *
+ * @param Doctrine\Common\Cache\Cache $cacheDriver
+ */
+ public function setCacheDriver($cacheDriver)
@stof

stof Oct 26, 2011

Member

shouldn't it be type-hinted ?

Member

schmittjoh commented Oct 27, 2011

You might want to consider using my metadata library as there is some code in common, and it has some more features which might come in handy in the future (like merging metadata, lazy-loading drivers, metadata on interfaces).

Another improvement would be to extract the finding logic to a separate class and inject that because right now if you want to customize this, you need to duplicate the logic in all drivers instead of just one place (see https://github.com/schmittjoh/metadata/blob/master/src/Metadata/Driver/AbstractFileDriver.php#L14).

@stloyd stloyd referenced this pull request in symfony/symfony Oct 27, 2011

Merged

[WIP] [Doctrine] Remove abtract doctrine bundle #2484

Owner

beberlei commented Oct 27, 2011

@schmittjoh that migration path is too steep for now. I will just rip the ORM mapping core out, which will be much easier to cope for in the first refactoring round.

@schmittjoh merging over different types (xml, annotation, yaml?)

Member

schmittjoh commented Oct 28, 2011

@henrikbjorn, yeah it doesn't matter what source the data is coming from.

@beberlei, yeah no need to rush this. However, extracting the _findMappingFile() method to a dedicated class seems doable no? It would save some code duplication, e.g. we wouldn't need a simplified driver, but only a different file locator.

Then metadata integration would be useful as u can only use one kind of mapping right now i think

fabpot added a commit to symfony/symfony that referenced this pull request Nov 1, 2011

merged branch beberlei/RemoveAbtractDoctrineBundle (PR #2484)
Commits
-------

661421f [Doctrine] Remove AbstractDoctrineBundle and move code into Doctrine Bridge

Discussion
----------

[WIP] [Doctrine] Remove abtract doctrine bundle

Remove AbstractDoctrineBundle and move code into Doctrine Bridge. It is a BC break because all the "other" Doctrine Bundles MongoDB ODM, CouchDB ODM and PHPCR need to be updated to cope with this.

I will prepare PRs for them aswell and then remove the [WIP] here.

Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #2463

---------------------------------------------------------------------------

by beberlei at 2011/10/26 12:32:51 -0700

Here are all 3 PRs, can we coordinate on merging them somehow?

https://github.com/symfony/DoctrineMongoDBBundle/pull/50
symfony-cmf/symfony-cmf#118
doctrine/DoctrineCouchDBBundle#4

---------------------------------------------------------------------------

by beberlei at 2011/10/26 12:33:38 -0700

Ping @lsmith77 @jwage

---------------------------------------------------------------------------

by lsmith77 at 2011/10/26 12:35:29 -0700

all good for me ..

---------------------------------------------------------------------------

by stof at 2011/10/26 14:58:01 -0700

Well, this does not fix #2463. A change done in the bridge will still be able to break the service definitions of the other bundles or require tricky stuff to keep different versions of the logic.

---------------------------------------------------------------------------

by beberlei at 2011/10/26 22:49:39 -0700

@stof true, that is what doctrine/common#71 will be about.

---------------------------------------------------------------------------

by stloyd at 2011/10/26 23:51:25 -0700

Comment just for linking cross PRs and for watching ;-) doctrine/common#71

---------------------------------------------------------------------------

by lsmith77 at 2011/10/31 08:18:45 -0700

please add forward compatibly into symfony 2.0 as per #2522

---------------------------------------------------------------------------

by beberlei at 2011/11/01 05:11:34 -0700

This doesn't make sense imho, since the number of people using the doctrine extension is 4 and everybody also prepared their pull request for this.

The problem is really that we need a branch for the MongoDB Bundle

---------------------------------------------------------------------------

by beberlei at 2011/11/01 05:40:49 -0700

@fabpot i created a branch in MongoDBBundle for 2.0 so this is ready to be merged.

beberlei pushed a commit to doctrine/DoctrineBundle that referenced this pull request Nov 17, 2011

merged branch beberlei/RemoveAbtractDoctrineBundle (PR #2484)
Commits
-------

661421f [Doctrine] Remove AbstractDoctrineBundle and move code into Doctrine Bridge

Discussion
----------

[WIP] [Doctrine] Remove abtract doctrine bundle

Remove AbstractDoctrineBundle and move code into Doctrine Bridge. It is a BC break because all the "other" Doctrine Bundles MongoDB ODM, CouchDB ODM and PHPCR need to be updated to cope with this.

I will prepare PRs for them aswell and then remove the [WIP] here.

Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #2463

---------------------------------------------------------------------------

by beberlei at 2011/10/26 12:32:51 -0700

Here are all 3 PRs, can we coordinate on merging them somehow?

https://github.com/symfony/DoctrineMongoDBBundle/pull/50
symfony-cmf/symfony-cmf#118
doctrine/DoctrineCouchDBBundle#4

---------------------------------------------------------------------------

by beberlei at 2011/10/26 12:33:38 -0700

Ping @lsmith77 @jwage

---------------------------------------------------------------------------

by lsmith77 at 2011/10/26 12:35:29 -0700

all good for me ..

---------------------------------------------------------------------------

by stof at 2011/10/26 14:58:01 -0700

Well, this does not fix #2463. A change done in the bridge will still be able to break the service definitions of the other bundles or require tricky stuff to keep different versions of the logic.

---------------------------------------------------------------------------

by beberlei at 2011/10/26 22:49:39 -0700

@stof true, that is what doctrine/common#71 will be about.

---------------------------------------------------------------------------

by stloyd at 2011/10/26 23:51:25 -0700

Comment just for linking cross PRs and for watching ;-) doctrine/common#71

---------------------------------------------------------------------------

by lsmith77 at 2011/10/31 08:18:45 -0700

please add forward compatibly into symfony 2.0 as per #2522

---------------------------------------------------------------------------

by beberlei at 2011/11/01 05:11:34 -0700

This doesn't make sense imho, since the number of people using the doctrine extension is 4 and everybody also prepared their pull request for this.

The problem is really that we need a branch for the MongoDB Bundle

---------------------------------------------------------------------------

by beberlei at 2011/11/01 05:40:49 -0700

@fabpot i created a branch in MongoDBBundle for 2.0 so this is ready to be merged.

fabpot added a commit to symfony/doctrine-bridge that referenced this pull request Nov 18, 2011

merged branch beberlei/RemoveAbtractDoctrineBundle (PR #2484)
Commits
-------

661421f [Doctrine] Remove AbstractDoctrineBundle and move code into Doctrine Bridge

Discussion
----------

[WIP] [Doctrine] Remove abtract doctrine bundle

Remove AbstractDoctrineBundle and move code into Doctrine Bridge. It is a BC break because all the "other" Doctrine Bundles MongoDB ODM, CouchDB ODM and PHPCR need to be updated to cope with this.

I will prepare PRs for them aswell and then remove the [WIP] here.

Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #2463

---------------------------------------------------------------------------

by beberlei at 2011/10/26 12:32:51 -0700

Here are all 3 PRs, can we coordinate on merging them somehow?

https://github.com/symfony/DoctrineMongoDBBundle/pull/50
symfony-cmf/symfony-cmf#118
doctrine/DoctrineCouchDBBundle#4

---------------------------------------------------------------------------

by beberlei at 2011/10/26 12:33:38 -0700

Ping @lsmith77 @jwage

---------------------------------------------------------------------------

by lsmith77 at 2011/10/26 12:35:29 -0700

all good for me ..

---------------------------------------------------------------------------

by stof at 2011/10/26 14:58:01 -0700

Well, this does not fix #2463. A change done in the bridge will still be able to break the service definitions of the other bundles or require tricky stuff to keep different versions of the logic.

---------------------------------------------------------------------------

by beberlei at 2011/10/26 22:49:39 -0700

@stof true, that is what doctrine/common#71 will be about.

---------------------------------------------------------------------------

by stloyd at 2011/10/26 23:51:25 -0700

Comment just for linking cross PRs and for watching ;-) doctrine/common#71

---------------------------------------------------------------------------

by lsmith77 at 2011/10/31 08:18:45 -0700

please add forward compatibly into symfony 2.0 as per #2522

---------------------------------------------------------------------------

by beberlei at 2011/11/01 05:11:34 -0700

This doesn't make sense imho, since the number of people using the doctrine extension is 4 and everybody also prepared their pull request for this.

The problem is really that we need a branch for the MongoDB Bundle

---------------------------------------------------------------------------

by beberlei at 2011/11/01 05:40:49 -0700

@fabpot i created a branch in MongoDBBundle for 2.0 so this is ready to be merged.

you should use assertContains to have a better failing message

+ * Whether the class with the specified name is transient. Only non-transient
+ * classes, that is entities and mapped superclasses, should have their metadata loaded.
+ * A class is non-transient if it is annotated with either @Entity or
+ * @MappedSuperclass in the class doc block.
@asm89

asm89 Nov 27, 2011

Member

This docblock should be revisited since it's to ORM specific.

+ /**
+ * Whether the class with the specified name should have its metadata loaded.
+ * This is only the case if it is either mapped as an Entity or a
+ * MappedSuperclass.
@asm89

asm89 Nov 27, 2011

Member

Docblock needs to be rewritten.

+ /**
+ * Whether the class with the specified name should have its metadata loaded.
+ * This is only the case if it is either mapped as an Entity or a
+ * MappedSuperclass.
@asm89

asm89 Nov 27, 2011

Member

Same here.

+ /**
+ * Whether the class with the specified name should have its metadata loaded.
+ *
+ * This is only the case for non-transient classes either mapped as an Entity or MappedSuperclass.
@asm89

asm89 Nov 27, 2011

Member

Same here.

+
+ /**
+ * {@inheritDoc}
+ * @todo Same code exists in AnnotationDriver, should we re-use it somehow or not worry about it?
@asm89

asm89 Nov 27, 2011

Member

Maybe look at this @todo while doing this big refactoring?

+/**
+ * The Symfony File Locator makes a simplifying assumptions compared
+ * to the DefaultFileLocator. By assuming paths only contain entities of a certain
+ * namespace the mapping files consinst of the short classname only.
@asm89

asm89 Nov 27, 2011

Member

typo consists + entity

Member

asm89 commented Nov 27, 2011

+1 on this, great enhancement.

+
+ $iterator = new \RecursiveIteratorIterator(
+ new \RecursiveDirectoryIterator($path),
+ \RecursiveIteratorIterator::LEAVES_ONLY
@stof

stof Nov 27, 2011

Member

the indentation is weird here

beberlei added a commit that referenced this pull request Nov 28, 2011

Merge pull request #71 from doctrine/AbstractMetadata
Extracted Abstract Mapping Driver & ClassMetadataFactory code from the ORM to be generically used by all Persistence implementations.

@beberlei beberlei merged commit 9ad3d51 into master Nov 28, 2011

Member

lsmith77 commented Nov 28, 2011

i guess i will wait for a CouchDB ODM update to see what needs to be done in PHPCR ODM

@stof stof deleted the AbstractMetadata branch Dec 17, 2014

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