-
Notifications
You must be signed in to change notification settings - Fork 34
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
Avoid empty string for scope when inserting/updating #563
Conversation
…eplaces it with null Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
8dd4d43
to
f940c03
Compare
Simpler and better now, right? |
lib/Db/Provider.php
Outdated
#[\ReturnTypeWillChange] | ||
public function jsonSerialize() { | ||
return [ | ||
'id' => $this->id, | ||
'identifier' => $this->identifier, | ||
'clientId' => $this->clientId, | ||
'discoveryEndpoint' => $this->discoveryEndpoint, | ||
'scope' => $this->scope, | ||
'scope' => trim($this->scope), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'scope' => trim($this->scope), | |
'scope' => trim($this->getScope()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also move the trim inside the getter then ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think we can't. It seems when updating/inserting a row in the DB, the value that is put there is accessed via the getter. So if we trim there, we will insert/update with an empty string which is what we want to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, fair enough then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why do you think we should use the getter in jsonSerialize? For empty scopes, the getter will replace it by a space and then we trim it anyway.
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Because oracle replaces it with null.
We could do it differently by allowing null values for the scope column. Any preferred option?