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

[DDC-3107][Persister] Remove the insertSql cache #1024

Closed
wants to merge 1 commit into from

Conversation

tyx
Copy link

@tyx tyx commented Apr 30, 2014

Here is a piece of code which leads to an exception if we keep it :
https://gist.github.com/tyx/353de6d6aaf367abd6e6

Sure it is an edge case with tricks but I'm sure Doctrine does not need this kind of optimization.

My 2 cent

Here is a piece of code which leads to an exception if we keep it

https://gist.github.com/tyx/353de6d6aaf367abd6e6

Sure it is an edge case with tricks but I'm sure Doctrine does not need this kind of optimization.

My 2 cent
@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3107

We use Jira to track the state of pull requests and the versions they got
included in.

@tyx tyx changed the title [Persister] Remove the insertSql cache [DDC-3107][Persister] Remove the insertSql cache Apr 30, 2014
@Ocramius
Copy link
Member

@tyx can you convert that example into a unit test? We can't really remove the insert sql cache, as it would impact performance quite a bit. Instead, we can see if we can instantiate a new persister per row (depending on the use case)

@tyx
Copy link
Author

tyx commented Apr 30, 2014

Yes you're right. I will do that.

I spent quite a lot of time to find the origin of this problem so I admit my solution could look radical ;)

@guilhermeblanco
Copy link
Member

Modifying ID Generation at runtime is weird and source of errors. You just hit one of them.

-1 on this change.

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

Successfully merging this pull request may close these issues.

4 participants