Conversation
Now requires Icinga/icingaweb2#5480
Al2Klimov
left a comment
Al2Klimov
left a comment
There was a problem hiding this comment.
Test
- Install Fedora 42
- Apply https://github.com/Al2Klimov/ansible-icinga-fedora
yum remove icingaweb2-module-monitoring- Downgrade to https://git.icinga.com/packages/icingaweb/-/jobs/856109
tar -C /usr/share/icingaweb2/modules -vxzf ~fedora/test5480.tgzicingacli mod en test5480- Visit /icingaweb2/test5480
- Input something
- Press enter
cat /etc/icingaweb2/modules/test5480/config.ini- Ok
chmod 0440 /etc/icingaweb2/modules/test5480/config.ini- Repeat steps 7-9
- Fails
Seems to work. So far – so good.
| t('Saving Configuration Failed!'), | ||
| )); | ||
|
|
||
| $this->addHtml(HtmlElement::create( |
There was a problem hiding this comment.
I wonder whether multiple individual HtmlElements are necessary...
There was a problem hiding this comment.
ShowConfiguration perfectly mimics the old showConfiguration.phtml. (Except adding the copy to clipboard button)
| $this->config->getConfigFile(), | ||
| $this->exception->getMessage(), | ||
| ), | ||
| HtmlString::create('<br>'), |
There was a problem hiding this comment.
I'd use multiple p instead of br.
@flourish86 What do you think?
There was a problem hiding this comment.
ShowConfiguration perfectly mimics the old showConfiguration.phtml. (Except adding the copy to clipboard button)
But yes, I'm totally open for a new design and/layout and an HTML rework.
| $value = $this->getConfigValue($element->getName()); | ||
|
|
||
| $configSection = $this->config->getSection($section); | ||
| if (empty($value)) { |
There was a problem hiding this comment.
php -r 'echo (int) empty("0");' says "1", so empty() seems not the best idea, does it?
There was a problem hiding this comment.
I have no problems with storing "0" values in the configuration.
Since it could be any type of element here, I don't think there is a reliable way determine if the field should be included or not.
| if ($this->isCreateForm()) { | ||
| $this->section = $this->getValue(static::NAME_ELEMENT_NAME); | ||
|
|
||
| if (empty($this->section)) { |
There was a problem hiding this comment.
I have no idea about this section stuff, but it sounds interesting...
There was a problem hiding this comment.
It is pretty common for configuration files that manage multiple identical instances of something (resources, users, pdf print backends, etc.).
So this new ConfigForm supports being pointed to a specific section and handling just that. (updaing and deleting)
It also can be used to create a new section.
An alternative way to having all three possible use cases in one form would be to split it up into two. Something like ConfigForm for the whole INI-file and ConfigSectionForm That focuses just one section, possibly creating or deleting it.
There was a problem hiding this comment.
Kind of a reference implementation would be helpful for testing.
There was a problem hiding this comment.
There already is an implementation this class in "section mode" in Icinga/icingaweb2-module-pdfexport#88
TheSyscall
force-pushed
the
config-form-5479
branch
from
4ab944a to
13eff40
Compare
Now requires Icinga/icingaweb2#5480
2 participants
An implementation of an INI based configuration form for CompatForms.
It allows developers to create a configuration form for an INI file and have it automatically populate form elements and store the results back into the specified file or section.
If writing the configuration file fails, an error is displayed alongside the full contents of the file to copy and paste manually by the admin.
This form can optionally be used to delete a section of the configuration file or create a new one.
As suggested here
resolves #5479