Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #5684 - Fix mapping on insertion for autoincrement columns with custom types #5935

Closed
wants to merge 6 commits into from

Conversation

mantiz
Copy link
Contributor

@mantiz mantiz commented Jul 14, 2016

Fixes #5684

The problem when using a custom type for autoincrement columns is that the value of autoincrement column returned by the database gets assigned directly without respecting mapping information. This only happens for insertion. When you fetch the records, the value is already mapped correctly.

@mantiz
Copy link
Contributor Author

mantiz commented Jul 18, 2016

Ok, it turned out to be a bigger problem than I initially thought 😉

I'm not sure if everything is covered but it looks good to me. Let me know if I forgot something or in the case a test case is missing.

I don't know what I think about the requirement to add a __toString() method to the custom type which is required for e.g. https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L1590. At least, it seems to work and it's easy to implement. 😄

@mantiz mantiz closed this Jul 20, 2016
@mantiz mantiz reopened this Jul 20, 2016
@Ocramius
Copy link
Member

@lcobucci can you check over this? Is it similar to #6020?

@lcobucci
Copy link
Member

@Ocramius sure, I think the changes made in #6020 are a bit simpler (can still be improved). This evening I'll check out the other branch and see if these tests are passing and provide feedback.

@mantiz
Copy link
Contributor Author

mantiz commented Nov 27, 2016

I visually compared this PR to #6020 and I think they both are effectively the same but the implementation in #6020 looks much cleaner than mine. So, I vote for closing this PR in favour of #6020.

@lcobucci
Copy link
Member

@mantiz I've combined your test case with @renan's code and rebased it with master on https://github.com/lcobucci/doctrine2/commits/fix/id-generator-custom-type. I think that the combination of the two PRs is the best thing to the project. Just waiting for @Ocramius opinion

@Ocramius
Copy link
Member

Handled in #6152

@Ocramius Ocramius self-assigned this Nov 27, 2016
@Ocramius Ocramius added the Bug label Nov 27, 2016
@Ocramius Ocramius added this to the 2.6.0 milestone Nov 27, 2016
@Ocramius
Copy link
Member

@mantiz your tests were merged into #6152, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants