#5804 Versioning doesn't work with a custom type primary key #6485

alle · 2017-06-05T09:51:44Z

This PR fixes #5804.
It's not a my own solution, but the one provided by @ngrevet-lightspeed.
Since I need this fixed, I made this PR on behalf of @ngrevet-lightspeed.

ngrevet · 2017-06-05T14:59:20Z

Thanks for taking the time to work my code into a PR, I'm not well versed in PRs on open source projects.

Ocramius

@alle the patch needs tests to be validated

alle · 2017-06-06T15:51:12Z

@Ocramius @ngrevet-lightspeed Everything should be fine, now. I was too hurried, last time. Sorry.

Ocramius · 2017-06-06T15:59:01Z

@alle I still don't see test changes though :-\

alle · 2017-06-06T16:11:57Z

@Ocramius Well, I thought tests shouldn't change, should they?
I thought I fixed this bug, working on the BasicEntityPersister::fetchVersionValue() method, by passing the involved tests, without changing them. Humbly, am I missing something?

Ocramius · 2017-06-06T16:13:39Z

@alle a test demonstrating the original bug (failing with the original repository state, passing with this patch) is needed ;-)

alle · 2017-06-12T14:03:41Z

@Ocramius: @astepin did the job (in #6496) that I should have done from myself.
@astepin The only change I made is in the custom type getSQLDeclaration() method: your PR was failing against the Postgres platform, since Doctrine maps to a specific datatype for GUIDs on that platform.

alle · 2017-06-14T14:55:09Z

I changed the involved classes in tests, complying to other tests' structure, in order to avoid a merge conflict on the SchemaValidatorTest, due to recent updates.

lcobucci · 2017-06-20T05:16:43Z

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

@@ -344,9 +344,16 @@ protected function fetchVersionValue($versionedClass, array $id)
             . ' FROM '  . $tableName
             . ' WHERE ' . implode(' = ? AND ', $identifier) . ' = ?';

+        $types = [];
+        foreach ($id as $field => $value) {
+            foreach ($this->getTypes($field, $value, $versionedClass) as $type) {


Do we need to iterate this too? I mean, in theory we could use array_merge() right, like:

$types = []; foreach ($id as $field => $value) { $types = array_merge($types, $this->getTypes($field, $value, $versionedClass)); }

@lcobucci You're simply right.

alle · 2017-07-06T09:23:21Z

@Ocramius @lcobucci Is there any possibility to have this PR merged? I didn't see any news since the last changes.

alcaeus · 2017-07-06T11:59:34Z

tests/Doctrine/Tests/Models/GH5804/GH5804Article.php

+     * @Column(type="GH5804Type")
+     * @GeneratedValue(strategy="CUSTOM")
+     * @CustomIdGenerator(class="\Doctrine\Tests\Models\GH5804\GH5804Generator")
+    ")


Syntax error ;)

Aaargh! Didn't see that!

alcaeus · 2017-07-06T11:59:53Z

tests/Doctrine/Tests/Models/GH5804/GH5804Article.php

+     * @Id
+     * @Column(type="GH5804Type")
+     * @GeneratedValue(strategy="CUSTOM")
+     * @CustomIdGenerator(class="\Doctrine\Tests\Models\GH5804\GH5804Generator")


Could/Should use ::class constant

In which form? Any example?

Paired with a use statement: @CustomIdGenerator(class=GH5804Generator::class). Works with Annotations >= 1.2 IIRC.

I guessed it was that way, but my local tests fail. I even update merged with the latest master release, updated the dependencies (my Annotations version was right, anyway), but your suggestion (which seems to be logical) doesn't work to me. Did you try by yourself?
Anyway, even without that ::class notation, everything is fine.

That's strange. I've been using that for (what feels like) a couple of years now.

It requires the FQCN, just that 😉

lcobucci · 2017-07-22T18:50:56Z

I'll take a look at this in a bit

lcobucci · 2017-07-22T20:30:01Z

@alle thanks for your contribution, I've simplified things a bit (kept all test related classes together).

@Ocramius it LGTM, can you check this one to see if we're good to go?

Ocramius

Some nitpicks apply

Ocramius · 2017-07-22T20:35:07Z

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

@@ -344,9 +344,14 @@ protected function fetchVersionValue($versionedClass, array $id)
             . ' FROM '  . $tableName
             . ' WHERE ' . implode(' = ? AND ', $identifier) . ' = ?';

+        $types = [];


Can this block be moved to a private method?

Ocramius · 2017-07-22T20:36:00Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH5804Test.php

+     * @CustomIdGenerator(class=\Doctrine\Tests\ORM\Functional\Ticket\GH5804Generator::class)
+     */
+    public $id;
+    /**


Ocramius · 2017-07-22T20:36:09Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH5804Test.php

+
+/**
+ * @Entity
+ * @Table(name="gh5804_articles")


Not really needed

lcobucci · 2017-07-22T21:29:34Z

@alle 🚢 thanks!

alle · 2017-07-24T08:09:16Z

Thank you so much, guys!
This was my first opportunity to contribute to this project, even to "calibrate" myself to future contributions. Or at least I hope so!

Ocramius added Bug Missing Tests labels Jun 5, 2017

Ocramius suggested changes Jun 5, 2017

View reviewed changes

astepin mentioned this pull request Jun 9, 2017

#5804 #6485 Versioning doesn't work with a custom type primary key #6496

Closed

lcobucci reviewed Jun 20, 2017

View reviewed changes

alcaeus reviewed Jul 6, 2017

View reviewed changes

lcobucci self-assigned this Jul 22, 2017

lcobucci removed the Missing Tests label Jul 22, 2017

Alessandro Frangioni added 5 commits July 22, 2017 22:26

Fixes #5804

af3591f

Inferring id's field types

4ef0a23

Applied patches as for #6496

f6907b9

Changed SQL declaration for custom type

82c8708

Iteration simplified

60a2628

lcobucci approved these changes Jul 22, 2017

View reviewed changes

Ocramius changed the title ~~Fixes #5804~~ #5804 Versioning doesn't work with a custom type primary key Jul 22, 2017

Ocramius added this to the 2.6.0 milestone Jul 22, 2017

Ocramius approved these changes Jul 22, 2017

View reviewed changes

lcobucci added 2 commits July 22, 2017 22:39

Move identifier types extraction to a method

eba8fec

Remove unnecessary annotation from test

6bf9f6f

lcobucci merged commit 9826d9c into doctrine:master Jul 22, 2017

alle deleted the versionWithCustomTypePrimaryKey branch July 24, 2017 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5804 Versioning doesn't work with a custom type primary key #6485

#5804 Versioning doesn't work with a custom type primary key #6485

alle commented Jun 5, 2017

ngrevet commented Jun 5, 2017

Ocramius left a comment

alle commented Jun 6, 2017

Ocramius commented Jun 6, 2017

alle commented Jun 6, 2017

Ocramius commented Jun 6, 2017

alle commented Jun 12, 2017 •

edited

Loading

alle commented Jun 14, 2017 •

edited

Loading

lcobucci Jun 20, 2017 •

edited

Loading

alle Jun 20, 2017

alle commented Jul 6, 2017

alcaeus Jul 6, 2017

alle Jul 6, 2017

alcaeus Jul 6, 2017

alle Jul 6, 2017

alcaeus Jul 6, 2017

alle Jul 6, 2017

alcaeus Jul 6, 2017 •

edited

Loading

lcobucci Jul 22, 2017

lcobucci commented Jul 22, 2017

lcobucci commented Jul 22, 2017

Ocramius left a comment

Ocramius Jul 22, 2017

Ocramius Jul 22, 2017

Ocramius Jul 22, 2017

lcobucci commented Jul 22, 2017

alle commented Jul 24, 2017

#5804 Versioning doesn't work with a custom type primary key #6485

#5804 Versioning doesn't work with a custom type primary key #6485

Conversation

alle commented Jun 5, 2017

ngrevet commented Jun 5, 2017

Ocramius left a comment

Choose a reason for hiding this comment

alle commented Jun 6, 2017

Ocramius commented Jun 6, 2017

alle commented Jun 6, 2017

Ocramius commented Jun 6, 2017

alle commented Jun 12, 2017 • edited Loading

alle commented Jun 14, 2017 • edited Loading

lcobucci Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alle commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci commented Jul 22, 2017

lcobucci commented Jul 22, 2017

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci commented Jul 22, 2017

alle commented Jul 24, 2017

alle commented Jun 12, 2017 •

edited

Loading

alle commented Jun 14, 2017 •

edited

Loading

lcobucci Jun 20, 2017 •

edited

Loading

alcaeus Jul 6, 2017 •

edited

Loading