Support custom id generators #206

Merged
merged 22 commits into from Mar 22, 2012

Conversation

Projects
None yet
4 participants
Contributor

velovint commented Nov 29, 2011

No description provided.

@stof stof commented on the diff Nov 29, 2011

tests/Doctrine/Tests/OrmTestCase.php
@@ -17,7 +17,7 @@ abstract class OrmTestCase extends DoctrineTestCase
/**
* @param array $paths
- * @return \Doctrine\Common\Annotations\AnnotationReader
+ * @return \Doctrine\ORM\Mapping\Driver\AnnotationDriver
@stof

stof Nov 29, 2011

Member

please revert it

@velovint

velovint Nov 29, 2011

Contributor

please explain why? do I miss something? that method returns AnnotationDriver and not reader

@stof

stof Nov 29, 2011

Member

ah right. I missed the fact that it said the annotation reader by reading too fast. I was just looking at the new code putting the base drivers in Common at the same time and thought it was a reverting of a recent change.

@stof stof commented on the diff Nov 29, 2011

...s/Doctrine/Tests/ORM/Mapping/PHPMappingDriverTest.php
// for the same mapping information
- $cme = new ClassMetadataExporter();
- $cme->addMappingSource(__DIR__ . DIRECTORY_SEPARATOR . 'yaml');
-
- $exporter = $cme->getExporter('php', $path);
- $exporter->setMetadatas($cme->getMetadatas());
- $exporter->export();
- */
+// $meta = new \Doctrine\ORM\Mapping\ClassMetadataInfo("Doctrine\Tests\ORM\Mapping\Animal");
+// $driver = $this->createAnnotationDriver();
+// $driver->loadMetadataForClass("Doctrine\Tests\ORM\Mapping\Animal", $meta);
+// $exporter = $cme->getExporter('php', $path);
+// echo $exporter->exportClassMetadata($meta);
@stof

stof Nov 29, 2011

Member

what is the point of replacing some commented code by some other commented code ?

@velovint

velovint Nov 29, 2011

Contributor

I need to generate PHP mapping from existing one and original code didn't work as interface of ClassMetadataExported changed. So I placed a code that can be used to convert mapping

@stof stof commented on an outdated diff Nov 29, 2011

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -1807,6 +1828,14 @@ class ClassMetadataInfo implements ClassMetadata
{
$this->idGenerator = $generator;
}
+
+ /**
+ * Sets definition
+ * @param array $definition
+ */
+ public function setCustomGeneratorDefinition(array $definition) {
@stof

stof Nov 29, 2011

Member

the curly brace should be on its own line (same for all other method definitions in your patch)

@stof stof commented on an outdated diff Nov 29, 2011

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -1807,6 +1828,14 @@ class ClassMetadataInfo implements ClassMetadata
{
$this->idGenerator = $generator;
}
+
@stof

stof Nov 29, 2011

Member

please remove the trailing whitespaces (same in other blank lines of your patch)

@stof stof commented on an outdated diff Nov 29, 2011

lib/Doctrine/ORM/Mapping/CustomIdGenerator.php
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the LGPL. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\ORM\Mapping;
+
+/**
+ * @Annotation
+ * @Target("PROPERTY")
+ */
+final class CustomIdGenerator implements Annotation {
@stof

stof Nov 29, 2011

Member

the curly brace should be on its own line

@stof stof commented on an outdated diff Nov 29, 2011

lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php
@@ -191,6 +191,12 @@ class YamlDriver extends AbstractFileDriver
// Check for SequenceGenerator/TableGenerator definition
if (isset($idElement['sequenceGenerator'])) {
$metadata->setSequenceGeneratorDefinition($idElement['sequenceGenerator']);
+ } else if (isset($idElement['customIdGenerator'])) {
+ $customGenerator = $idElement['customIdGenerator'];
+ $metadata->setCustomGeneratorDefinition(array(
+ 'class' => (string) $customGenerator['class'],
+ 'args' => $customGenerator['args']
+ ));
@stof

stof Nov 29, 2011

Member

the indentation is wrong here

@stof stof and 1 other commented on an outdated diff Nov 29, 2011

...trine/Tests/ORM/Mapping/AbstractMappingDriverTest.php
@@ -88,6 +88,18 @@ abstract class AbstractMappingDriverTest extends \Doctrine\Tests\OrmTestCase
$class->sequenceGeneratorDefinition
);
}
+
@stof

stof Nov 29, 2011

Member

trailing whitespaces should be removed

@velovint

velovint Nov 29, 2011

Contributor

Thanks for your feedback. I fixed whitespaces and brace locations in 353ba4d.

@stof stof commented on an outdated diff Nov 29, 2011

...ctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php
@@ -64,10 +48,64 @@ class ClassMetadataFactoryTest extends \Doctrine\Tests\OrmTestCase
$this->assertEquals(ClassMetadata::GENERATOR_TYPE_SEQUENCE, $cm1->generatorType);
}
+ public function testGetMetadataFor_ReturnsLoadedCustomIdGenerator() {
@stof

stof Nov 29, 2011

Member

the curly brace should be on its own line (same for next methods)

Contributor

velovint commented Dec 8, 2011

guys, do you have any concerns or proposals on this functionality?

Please remove this. It does not exist anymore in Doctrine codebase.

Owner

velovint replied Dec 20, 2011

Do you mean AnnotationRegistry::registerFile ? It's already removed in 6bcfaed

This file doesn't exist anymore. Please merge with master again

Owner

guilhermeblanco commented Dec 14, 2011

@velovint Please merge with current master.
I saw a couple of things outdated there.

Except that, I'm fine. @beberlei you fine too?

Owner

beberlei commented Dec 19, 2011

There is a change missing on ClassMetadata that adds the customIdGenerator of ClassMetadataInfo to the serialized fields and reconstructs it on wakeup.

Additionally i don't like the way additional arguments are passed to the generator. This is something that should be placed in a greater context of Dependency Injection which i plan for 2.3

Contributor

velovint commented Dec 20, 2011

@guilhermeblanco done. Have only 34 skipped test, the rest is passing fine.

@beberlei there is no ClassMetadataInfo::customIdGenerator. ID generator is stored in ClassMetadataInfo::idGenerator which is serialized. Generator definition is stored in customGeneratorDefinition and is not serialized. I simply followed Sequence Generator approach.

I agree with you that passing parameters this was isn't ideal, but I couldn't find a way to have usability of named arguments along with flexibility. Do you think current implementation of custom ID generator will complicate moving ID generator definitions inside of Dependency Injection context?

I think that having something more generic at the base of ID generators can simplify current implementation of other ID generators configuration.

Owner

beberlei commented Dec 20, 2011

@velovint If sequenceGenerator is not serialized, then this is a bug as well. Not serialized means, Doctrine forgets it in the first cached request.

Regarding DI, i just don't want this API now, since then i have to maintain it forever. Can we make it without parameters for now? (I hope thats still useful, however you can create a sub-class per configuration). Then we don't have to maintain two APIs.

Contributor

velovint commented Dec 23, 2011

@beberlei it's not sequenceGenerator or customIdGenerator not serialized, they both stored in idGenerator property which is serialized. Their definitions aren't serialized, however (sequenceGeneratorDefinition), but this shouldn't be a problem, as it's used only during initialization. So, do you want me to add generator definitions to the list of serialized properties?

I totally understand you concert regarding API. If we can have support without parameters this works for my purpose and others like "UUID Generator" as well. I'll modify it.

Owner

beberlei commented Dec 23, 2011

ah yes, now i get it with the properties, sometimes i miss the picture :)

Perfect with the refactoring. This probably makes the patch much simpler also.

Contributor

velovint commented Jan 8, 2012

@beberlei not sure if you get notifications when I push new commits. I removed support to pass arguments to generator's constructor reducing total changeset to "15 files changed, 245 insertions(+), 34 deletions(-)" from "295 insertions(+), 34 deletions(-)"

Owner

beberlei commented Jan 8, 2012

@velovint i get notifications, but i get so many of them and don't have infinite time ;)

Owner

beberlei commented Jan 28, 2012

Sorry that i didnt write back on this yet, this will be in 2.3, however i want to get 2.2 out before coming back to this ticket.

Contributor

velovint commented Jan 31, 2012

@beberlei thanks for update. please let me know when you have time to check it out, so that I can merge latest changes from master.

Contributor

velovint commented Feb 21, 2012

@beberlei I just merged in master and verified that all tests pass

Owner

beberlei commented Mar 11, 2012

During review i realized this has a bug, its not handling serialize/unserialize correctly and will not work in combination with a cache therefore.

@beberlei beberlei merged commit 78d3f64 into doctrine:master Mar 22, 2012

Contributor

velovint commented Mar 26, 2012

@beberlei sorry for delay. Is there still an issue with serialize/unserialize? If so how can I see it?

Owner

beberlei commented Mar 27, 2012

I fixed that myself.

On Tue, Mar 27, 2012 at 1:11 AM, Vitali Yakavenka <
reply@reply.github.com

wrote:

@beberlei sorry for delay. Is there still an issue with
serialize/unserialize? If so how can I see it?


Reply to this email directly or view it on GitHub:
#206 (comment)

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