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

Remove scheduleExtraUpdate calls #320

Open
wants to merge 1 commit into
base: master
from

Conversation

@EmmanuelVella
Copy link
Contributor

commented Aug 10, 2016

Issue

BlameableSubscriber (at least) is always doing an extra database update after inserting/updating an entity. It seems to be caused by the scheduleExtraUpdate calls.

This is especially annoying because I use doctrine version locking, and the version is incrementing 2 by 2.

Examples (simplified)

Insert

INSERT INTO my_table (created_by_id, updated_by_id) VALUES (?, ?) {"3":2,"4":2}
UPDATE my_table SET created_by_id = ?, version = version + 1 WHERE id = ? AND version = ? [2,503,1]

Update

UPDATE my_table SET updated_by_id = ?, version = version + 1 WHERE id = ? AND version = ? [2,502,6]
UPDATE my_table SET updated_by_id = ?, version = version + 1 WHERE id = ? AND version = ? [2,502,7]

Solution

Removing these calls fixes the issue : only 1 insert or update is performed, and the createdBy and updatedBy are inserted/updated correctly.

Examples (simplified)

Insert

INSERT INTO my_table (created_by_id, updated_by_id) VALUES (?, ?) {2, 2}

Update

UPDATE my_table SET updated_by_id = ?, version = version + 1 WHERE id = ? AND version = ? [2,502,2]

Conclusion

I'm not sure about the side effects of removing this call, please feel free to comment this PR !

@hectorh30

This comment has been minimized.

Copy link

commented Sep 6, 2016

I'm having an issue with these extra database calls too, mainly because of the logging behavior: I'm getting twice the amount of logs I'd expect.

As far as I could dig into, this behaviour was introduced in ca44b17, but I don't know how to find what was the first release to include the commit, so as to look for any reference.

Are there any side effects to removing the scheduled extra updates?

@docteurklein

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2016

thanks for the detailled explanation!

I wonder if we added these extra updates because of some edge cases.
I just don't remember so we'd need to add tests in order to avoid any regression.
Would you have time to add some tests?

@EmmanuelVella

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2016

Hey, I will try to add some tests in the next days. Thanks for feedback !

@EmmanuelVella

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2016

Hi, I won't have time to work on this at the moment. If someone want to create some tests, feel free to duplicate my PR ;)

@EmmanuelVella EmmanuelVella force-pushed the EmmanuelVella:scheduleExtraUpdate branch 2 times, most recently from d018cdf to 29d5252 Apr 5, 2018
@EmmanuelVella EmmanuelVella force-pushed the EmmanuelVella:scheduleExtraUpdate branch from 29d5252 to a036962 Apr 5, 2018
@EmmanuelVella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2018

Hello, after a long time I finally decided to update my PR with tests !

I fixed Blameable and Geocodable and also added tests for SoftDeletable (this one needs the scheduleExtraUpdate call).

Could someone please have a look ? Thank you !

@EmmanuelVella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2018

ping @docteurklein do you still work on this project ?

@EmmanuelVella

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

Hey there ;) Could someone please check (and merge?) this PR please ? Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.