console (cli): Add config:delete to delete a key #16576 #16608
Conversation
There was a problem hiding this comment.
Cheers for the PR @javier-de-juan very appreciated. Added 2 comments to check if we need certain features.
| To use arguments, supply one or more arguments in the following format: | ||
|
|
||
| Remove section: | ||
| $ ./console config:delete 'section' |
There was a problem hiding this comment.
@javier-de-juan do you need the feature to remove an entire section? Cause if used wrongly it could do a lot of harm and break Matomo (eg if someone removes General or any other sections and forgets to pass a key, then Matomo be broken probably). If you need this feature, we'd probably need another flag like only-section just to be sure this is wanted
There was a problem hiding this comment.
Hi @tsteur! In my case it's not needed but I tried to think in all cases. I can remove this feature and keep it simple. I'll do it ASAP, today will be done ;)
There was a problem hiding this comment.
I was thinking about this feature and, in my opinion, it could be useful. Finally I decided to put a confirm question to avoid undesired behaviour. What do you think???
There was a problem hiding this comment.
Not sure where it would be useful? Not sure there is any section that can be removed without breaking anything actually.
There was a problem hiding this comment.
Ok @tsteur The feature was also removed. Actually the command will only remove settings in sections 👍
| 'config_setting_key' the name of the setting | ||
|
|
||
| To remove an array setting position, supply an argument like this: | ||
| $ ./console config:set 'section.config_setting_key[position]' |
There was a problem hiding this comment.
@javier-de-juan in case you don't need this feature be great to remove it as well just as it adds a bit of complexity and people might rarely know the correct index of an array etc. Happy to keep it though if needed.
There was a problem hiding this comment.
The same with this, I could remove it. I'm going to wait for someone with different opinion and If there's no comments today I will change it and give just the posibility of remove settings in sections ;)
There was a problem hiding this comment.
Thanks. Be great to remove it and if needed it can be always added quickly. This way it's less things to maintain that otherwise maybe nobody would use etc and simplifies the documentation, the code etc. Let me know once you made the changes and we'll review 👍 Thanks for this, very appreciated.
There was a problem hiding this comment.
This feature was completely removed from the code ;)
javier-de-juan
force-pushed
the
4.x-dev
branch
from
4ff256a
to
17395db
Compare
|
@tsteur the original issue references having a |
|
@diosmosis good one. That could be useful indeed. @javier-de-juan I think previously one had to set a specific index to delete a value within an index. But the correct index is always hard to know. If it was possible to use |
|
Ok, there's no problem. But... what's the desired behavior? If 2 settings have the same value the command deletes both? The section is always required, optional? I need a description about what you need to avoid wrong implementation. |
|
I would say deleting value from an array would only work when specifying the key and the section as well. Basically When a value is set, and the configured value is a string (or a number) then we would only delete that value if the configured value matches the value from the If the configured value is an array, then we only remove that one value from the array that matches the value from the option. Meaning if
Does this help? |
|
@javier-de-juan be great to comment once you made the adjustments then we will do another review. Thanks for all this again |
|
@javier-de-juan are you maybe still working on this PR? |
sgiehl
added
the
Waiting for user feedback
sgiehl
added
Pull Request WIP
|
@javier-de-juan do you maybe have some time to apply the review feedback? Would be awesome if we could merge this for the next release. |
|
@javier-de-juan I will close this PR due to inactivity. If you are still keen on it feel free to let us know and we reopen the PR |
This PR tries to add the delete command for config file. For array positions deletion, just admit numeric number because if the code removes one labeled position from the array, all other keys are lost (dumpConfig method related), so I preferred to avoid bad config generation.