-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 whitespace from base-DNs #18484
Conversation
@@ -146,6 +146,7 @@ public function setConfiguration($config, &$applied = null) { | |||
} | |||
|
|||
$setMethod = 'setValue'; | |||
$trim = false; |
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.
indentation
@@ -276,14 +278,18 @@ protected function getMultiLine($varName) { | |||
* @param string $varName | |||
* @param array|string $value | |||
*/ | |||
protected function setMultiLine($varName, $value) { | |||
protected function setMultiLine($varName, $value, $trim = false) { |
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.
Please adjust PHPDoc
A new inspection was created. |
Thanks for the feedback. I've updated the file accordingly. Also, the logic has been changed so that trim also works on non-array values on |
Is it possible to add unit tests for these? |
Actually we can simplify it and work without the $trim parameter, just trim all the string settings. Trimming is always safe, except for the password. For this we can check in setValue() (ugly) or introduce another where we don't do any manipulation to strings like setUnmodifiedValue() or setRawValue() and apply this method for the password. This could be called from within setMultiLine as well to avoid an extra trim. |
@Takuto88 an update about #18484 (comment)? |
OK, I finished this as suggested in #18484 (comment) and added unit tests for setting the configuration items. @davitol @LukasReschke @MorrisJobke review and testing would be awesome |
Tested and works 👍 I rebased this on current master for easier testing. |
👍 WFM |
Remove whitespace from base-DNs
Potential regression: #19793 |
Fixes #17964.
I've tried to implement trimming in
Connection::doCriticalValidation()
as hinted in the issue. However, this does not work that way. Everything that is changed during that method call is somehow being reset to its original value afterwards. I am not sure why that is. As far as my understanding of the code goes, this should not happen.Because of this, I've opted to implement trim when saving these specific config values in
Configuration::setConfiguration()
which works reliably for me.cc @blizzz: Please review. Thanks