Validation of y/n answers in setlanip #1382
Conversation
At the moment the user can answer "yes" to most of the questions, but then later code only checks if the answer is "y". Thus you can type in "yes" in some places, have it accepted, but actually the negative action is taken. That is weird and will mess up people who try typing a whole string starting with "y". With this change it makes the user type one of "y", "yes", "n", "no". When they type 1 of those, it is turned into either "y" or "n". Then the existing implementation logic all works as expected. Also, it is not obvious when the last question is being asked, before actually implementing. IMHO it would be good to give the user a chance to bail out if they have answered questions wrongly and just want to get back and start again. So "Do you want to apply these changes?" question has been added. Also there was a bug in checking $intbits value at line 299 - I could enter a big number for CIDR and it accepted it and blew up later trying to implement something like 10.11.12.1/99
| $good_answer = false; | ||
|
|
||
| do { | ||
| echo "\n" . gettext($prompt_text) . " (y/n) "; |
There was a problem hiding this comment.
gettext() calls should happen on strings, not on variable, otherwise strings will never be added to PO file by xgettext
|
Ah yes, I wasn't thinking about that - I guess the gettext has to be done on the real text before it is passed in each call. Will do... |
|
I had not been thinking consistently! Some gettext calls I had left with the original text, and had also put the gettext() in the subroutine. |
| echo "\n" . gettext("Changes have not been applied.\n"); | ||
| fclose($fp); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Are you sure it's going to work? I was reading code above and it clearly touch $config (e.g. line 404). Users are going to believe all those changes were ignored when they answer no here, and it's not true, or did I miss something?
There was a problem hiding this comment.
I was thinking that the values in $config are just pulled in by the require_once() of config.inc
Then if they are never written back to config.xml (no write_config is done), they will just be effectively thrown away when the code returns, and thus any changes would be gone.
So maybe this is not true - maybe the caller rc.initial is going to now have a part-modified $config[] array and if some other mod is done later and write_config() is called, then these bits of changes will end up getting saved.
Global variables that are not read-only are are a pain!
I will have a think - I guess I can save up any changes in other vars and set them in $config after the user has confirmed.
|
Leave this one for a while. I have made pull request #1383 to fix the bug in validating CIDR that I noticed. If you can commit that, then I think I will make an subsequent pull request to commit the fixup of the (y/n) question handling, without the final confirmation question. That will get the existing code working nicely. |
At the moment the user can answer "yes" to most of the questions, but then later code only checks if the answer is "y". Thus you can type in "yes" in some places, have it accepted, but actually the negative action is taken. That is weird and will mess up people who try typing a whole string starting with "y".
With this change it makes the user type one of "y", "yes", "n", "no". When they type 1 of those, it is turned into either "y" or "n". Then the existing implementation logic all works as expected.
Also, it is not obvious when the last question is being asked, before actually implementing. IMHO it would be good to give the user a chance to bail out if they have answered questions wrongly and just want to get back and start again. So "Do you want to apply these changes?" question has been added.
Also there was a bug in checking $intbits value at line 299 - I could enter a big number for CIDR and it accepted it and blew up later trying to implement something like 10.11.12.1/99
Should address redmine #4100