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

[WIP] API: Don’t auto-convert update to an insert in DB::manipulate() #7073

Closed
wants to merge 1 commit into from
Closed

Conversation

sminnee
Copy link
Member

@sminnee sminnee commented Jun 28, 2017

This feature creates an extra SELECT on every update and its
only benefit is to paper over other bugs that should be fixed
with better data integrity.

Fixes #7072

This feature creates an extra SELECT on every update and its 
only benefit is to paper over other bugs that should be fixed
with better data integrity.

Fixes #7072
@@ -343,12 +343,8 @@ public function manipulate($manipulation)
$query->addWhere(array('"ID"' => $writeInfo['id']));
}

// Test to see if this update query shouldn't, in fact, be an insert
if ($query->toSelect()->count()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This by itself isn't going to be enough. The insert logic in DataObject::write() initially writes to the base table to get the ID, and that relies on the base table insert being converted to an update here.

I thought the fix was to use affectedRows() to trigger the fallback to update?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

affectedRows() seems to return false if you run an update query that doesn't change any columns.

Well, I know that it erroneously returns false sometimes, I'm guessing that that's the reason why.

Copy link
Member Author

@sminnee sminnee Jul 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case #7072 has more details.

Imma close this PR as it adds little value keeping it open.

@tractorcow tractorcow added this to the Recipe 4.0.0-beta2 milestone Jul 2, 2017
@sminnee sminnee changed the title API: Don’t auto-convert update to an insert in DB::manipulate() [WIP] API: Don’t auto-convert update to an insert in DB::manipulate() Jul 2, 2017
@sminnee sminnee closed this Jul 2, 2017
@sminnee sminnee removed this from the Recipe 4.0.0-beta2 milestone Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants