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

Bugfix for MultilanguageModel #674

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@tterhaarlaudert
Copy link
Contributor

commented Dec 7, 2018

A bug occurs while editing a content page in the VCMS for the 8th language.
SEO-URL for oxcontent can't be updated if all other values (title, content, etc.) stay unchanged.
Because no values have to be updated in the table "oxcontents_set1" the function executeDatabaseQuery returns 0 for the amount of affected rows. In this case the update function would return false although the content has been saved successfully.

Bugfix for MultilanguageModel
A bug occurs while editing a content page in the VCMS for the 8th language.
SEO-URL for oxcontent can't be updated if all other values (title, content, etc.) stay unchanged.
@CLAassistant

This comment has been minimized.

Copy link

commented Dec 7, 2018

CLA assistant check
All committers have signed the CLA.

@keywan-ghadami-oxid

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2018

@reviewer I did a code review and
from code it seams to be correct.

In general it is not correct to use return value of 'execute' to make assumptions about the success of the sql statement as it only returns the number of effected rows and that can be 0 if the value remains unchanged.

If there is really the need to know if there was a match in the database there is also the possibility to change that MySQL behavior to return always the amount of rows that match even if not changed by setting CLIENT_FOUND_ROWS in the connection https://dev.mysql.com/doc/refman/8.0/en/mysql-affected-rows.html.

In the BaseModel _update will always return true independent of the number of affected rows so I guess that behavior would also be correct for this class.

@Sieg Sieg added the in progress label Dec 19, 2018

@Sieg

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

Thank you @tterhaarlaudert for the fix, i have merged it recently to our b-6.1.x, so will be available with next patch. (3212ce1)

@Sieg Sieg closed this Dec 20, 2018

@Sieg Sieg removed the in progress label Dec 20, 2018

@Sieg Sieg self-assigned this Dec 20, 2018

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