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

Do not fail when config settings have trailing spaces #6442

Open
mattab opened this issue Oct 14, 2014 · 4 comments
Open

Do not fail when config settings have trailing spaces #6442

mattab opened this issue Oct 14, 2014 · 4 comments
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.

Comments

@mattab
Copy link
Member

mattab commented Oct 14, 2014

We had a problem that the config file contained value: session_save_handler = "dbtable ". There is an extra space after dbtable which was added by mistake and is hard to spot.

  • Expected: Piwik returns the value trimmed: dbtable
  • Got: the value with trailing space dbtable[ ]

Steps

  • In config, return value after trimming
@mattab mattab added the Bug For errors / faults / flaws / inconsistencies etc. label Oct 14, 2014
@mattab mattab added this to the Piwik 2.9.0 milestone Oct 14, 2014
@tsteur
Copy link
Member

tsteur commented Oct 14, 2014

Not sure if it is actually a good idea to trim all values. In the end this is a user mistake even if it is hard to spot. Alternatively it could be done in some places where surely no whitespace is expected.

Keep in mind there might be for instance also plugins etc that are using the config for their own values etc.

@diosmosis
Copy link
Member

We can have per-config setting validation when DI is added and used for config.

@ThaDafinser
Copy link
Contributor

Alternative: enhance the error/exception message with the needed information. Try their if with trim the string is not equal?

@tsteur
Copy link
Member

tsteur commented Oct 16, 2014

This could be a nice idea as well. We should convert (eg to integer) or trim keys automatically when we know it is safe. Showing a message that somewhere is a trailing or leading space where we know there should certainly not be one is not needed in this case. This should NOT be handled by the config class and for sure not for all keys. Instead this should be done in the code where needed. To avoid having duplicated code to convert a single config value we ideally create a getter method outside the config class where it belongs to. This is also easily testable then.

If something is suspicious about a value we should show a message or trigger an exception. For instance if a number is expected but got a string. This should not be converted to int automatically as it might be wrong.

@mattab mattab modified the milestones: Short term, Piwik 2.9.0 Oct 17, 2014
@mattab mattab added Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. and removed Bug For errors / faults / flaws / inconsistencies etc. labels Oct 17, 2014
@mattab mattab modified the milestones: Mid term, Short term Dec 1, 2014
@mattab mattab modified the milestones: Long term, Mid term Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

4 participants