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

EZP-29096: Do not store empty draft values in DB #1417

Merged
merged 12 commits into from
Mar 11, 2019

Conversation

pkamps
Copy link
Contributor

@pkamps pkamps commented Jan 17, 2019

The issue and steps to reproduce it are described here:
https://jira.ez.no/browse/EZP-29096

Or use those steps:

  • Create a new user account
  • Do not fill any values in the form
  • Close the browser tab
  • Try to create a new user account
  • You will get a transaction error

This patch will prevent the user creation to store any draft data into the DB unless the editor specified a user login.

@andrerom
Copy link
Contributor

andrerom commented Jan 18, 2019

Nice! 👍

We should aim to place this one 2017.12 and up. As for Travis, unsure why but tests seems to fail with:

.PHP Fatal error:  Call to a member function store() on null in /home/travis/build/ezsystems/ezpublish-legacy/kernel/classes/datatypes/ezuser/ezusertype.php on line 282

Copy link
Member

@glye glye left a comment

Choose a reason for hiding this comment

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

Agree on 2017.12, and we must figure out why that test breaks.

kernel/classes/datatypes/ezuser/ezuser.php Outdated Show resolved Hide resolved
kernel/classes/datatypes/ezuser/ezusertype.php Outdated Show resolved Hide resolved
@glye
Copy link
Member

glye commented Jan 21, 2019

onPublish() fails because there is no user object. Seems the code expects the draft to have been stored, so bigger changes are needed.
https://github.com/ezsystems/ezpublish-legacy/pull/1417/files#diff-7c45a8ca23a28df173a64ef5548180c0R282

@peterkeung
Copy link
Contributor

Are tests passing with the latest commit?

@glye
Copy link
Member

glye commented Jan 30, 2019

@peterkeung No, sorry. Don't you have access to see the results? Travis says eZUserTypeRegression::testUpdatePasswordUpdatesSerializedData The password hash stored in the user attribute should have been updated Failed asserting that null is not equal to null.

Copy link
Member

@glye glye left a comment

Choose a reason for hiding this comment

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

Tests pass! 🎉 🙌
Some CS issues and minor grumbles, and questions about other changes, in particular the getSerializedPasswordHash change.

kernel/classes/datatypes/ezuser/ezusertype.php Outdated Show resolved Hide resolved
// only if the object version is a draft (status == 0)
if(
$user->Login &&
$contentObjectAttribute->attribute( 'object_version' )->attribute( 'status' ) == 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$contentObjectAttribute->attribute( 'object_version' )->attribute( 'status' ) == 0
$contentObjectAttribute->attribute( 'object_version' )->attribute( 'status' ) === 0

Or can this be a string? I would not think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope you don't mind if I ignore it. Maybe it's a string and then it would hurt to be extra strict here.

Copy link
Member

Choose a reason for hiding this comment

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

We ought to be strict, strictly speaking ;) eZContentObjectVersion says it's an integer. But I see many cases where we aren't strict, so I guess this can pass if others agree.

kernel/classes/datatypes/ezuser/ezusertype.php Outdated Show resolved Hide resolved
kernel/classes/datatypes/ezuser/ezusertype.php Outdated Show resolved Hide resolved
kernel/user/ezuseroperationcollection.php Outdated Show resolved Hide resolved
@andrerom
Copy link
Contributor

@pkamps can you accept / reject @glye's fixes?

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

LGTM

@andrerom andrerom merged commit 1adb33d into ezsystems:master Mar 11, 2019
andrerom pushed a commit that referenced this pull request Mar 11, 2019
* EZP-29096: Do not store empty draft values in DB

(cherry picked from commit 1adb33d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants