fixes #525 using explicit default return value on nonexistent key #526
Conversation
| // return empty string as default value if no such key exists | ||
| if(0 == aConfig.count(name)) | ||
| { | ||
| return(""); |
There was a problem hiding this comment.
I think instead of returning "" this should error/throw an exception/abort the program or else it will be hard to spot typos
There was a problem hiding this comment.
returning "" is what the code previously did, implicitly, for config->val and config->p2, except that it did break for config->p1. it's merely explicit now. I could change it back to an implicit variant that also works for p1, but this seems to make the behaviour more obvious at least.
The problem with building in hard errors is that nothing in the code currently expects the program to break or throw when nonexistent keys are requested from the DB. Current behaviour everywhere is to assume a default numerical value of 0 if a key doesn't exist, which is what the empty string gets converted to.
Of course it is far from perfect, but this at least fixes my original issue without the need for re-engineering of the DB "interface" between frontend and backend and refactoring the whole backend codebase. Problem is, we don't have any specification on what either end is expecting to be done by the other end and who is responsible for doing what.
Eg. should there be a cronjob that writes default config values from the XML (which are displayed in the frontend/admintool) to the DB (which is the only place where the backend looks for the config values) regularily if one of the values doesn't exist?
There was a problem hiding this comment.
Right, I just figured as you were already cleaning this up, adding some kind of error would be helpful. If out of scope can you move this to an issue then one day someone might pick this up?
I guess for you this became an issue because the eventhandler was running and you never reset the config? To solve that, I think it might make most sense to set up a Symfony Command that resets the config and allows to set specific config values. Can then add that to vagrant provision. Long term goal for live versions would be to have some sort of health check that verifies this and alerts us if something is broken.
There was a problem hiding this comment.
good point. created #527 for this.
it would be great if you could make something in the frontend that does "write default values to the DB for all config parameters that don't have an entry in the DB yet". something that can be triggered from the admin tool and maybe from a cronjob would be nice.
There was a problem hiding this comment.
i don't have any symfony skills to do that frontend thing myself, so would really be appreciated if you could do that^^
or maybe I could hack together some php that does the check db->compare xml->write missing part and you integrate it properly into symfony?
underlying issue of existence assumptions for config values still exists, might lead to more unexpected behaviour in the future (but no more sql errors for now)