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

Adding new command config:set command to set INI config #9124

Merged
merged 9 commits into from Dec 18, 2015

Conversation

diosmosis
Copy link
Member

This PR includes a new command core:set-config which will set INI config. The command is meant to provide convenience and opportunities for automation.

There are two ways to set an INI config:

  1. You can use the --section, --key, --value options to set a single simple value. You cannot set non-string values or append to arrays w/ this option, but it may be simpler to use when using the command in scripts.
  2. You can use arguments formatted like Section.name="value" or Section.name[]="value" to set values and append values, respectively. Values must be JSON encoded, so arrays can be assigned.

CC @mattab

@diosmosis diosmosis added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Oct 29, 2015
@diosmosis diosmosis added this to the 2.15.1 milestone Oct 29, 2015
@diosmosis diosmosis added the Needs Review PRs that need a code review label Oct 29, 2015
{
protected function configure()
{
$this->setName('core:set-config');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't really use the core namespace. Maybe config:set or something similar would be better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested this along w/ core:setconfig, @mattab liked core:set-config.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should really avoid core. We already fixed this for some like core:clear-caches and renamed it to cache:clear, same for plugin:.... Apart from us core developer nobody really knows what we mean by core and it doesn't provide any real value to use this term @mattab

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config:set sounds good 👍

@mattab
Copy link
Member

mattab commented Nov 23, 2015

Another feedback:

  • add this new command to the CHANGELOG.md

@mattab mattab closed this Dec 3, 2015
@mattab mattab deleted the config_set_command branch December 3, 2015 21:56
@mattab mattab restored the config_set_command branch December 3, 2015 22:00
@mattab mattab reopened this Dec 3, 2015
@mattab
Copy link
Member

mattab commented Dec 4, 2015

@diosmosis I renamed the command to config:set and tested it, I notice it does not work for me, the config file is not updated... When adding $config->clear() in d1fc19d in the integration test this issue can be reproduced (ie. the config file was not updated). could you take a look?

@mattab mattab changed the title Adding new command core:set-config command to set INI config Adding new command config:set command to set INI config Dec 4, 2015
@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Dec 4, 2015
@diosmosis
Copy link
Member Author

@mattab fixed the issue & refactored a bit. you can review and/or merge.

@diosmosis diosmosis removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Dec 10, 2015
@mattab
Copy link
Member

mattab commented Dec 15, 2015

Feedback:

  • in help text it says NOTE: 'value' must be JSON encoded, so Section.config_setting_name="value" would work but Section.config_setting_name=value would not. but I found that it is actually working eg. ./console config:set --section=Tracker --key=debug --value=value2
    • should this text be removed?
  • One can add array values with ./console config:set 'Section.config_setting_name[]="value to append2"' but how does one reset an array to empty? let's add this to command help text + a test case
  • Assigning 0 value does not work, but it was expected to work:
$ ./console config:set --section=Tracker --key=debug --value="0"

  [InvalidArgumentException]                                                                        
  Nothing to assign. Add assignments as arguments or use the --section, --key and --value options.  

@diosmosis
Copy link
Member Author

n help text it says NOTE: 'value' must be JSON encoded, so Section.config_setting_name="value" would work but Section.config_setting_name=value would not. but I found that it is actually working eg. ./console config:set --section=Tracker --key=debug --value=value2

It only needs to be JSON encoded when using arguments, not options.

One can add array values with ./console config:set 'Section.config_setting_name[]="value to append2"' but how does one reset an array to empty? let's add this to command help text + a test case

./console config:set 'Section.config_setting_name=[]' should do it.

Assigning 0 value does not work, but it was expected to work:

Will look into it.

@diosmosis
Copy link
Member Author

@mattab FYI, fixed the issue + tweaked the docs + added more tests.

mattab pushed a commit that referenced this pull request Dec 18, 2015
Adding new command config:set command to set INI config
@mattab mattab merged commit 88e7546 into master Dec 18, 2015
@mattab mattab deleted the config_set_command branch December 18, 2015 02:39
@mattab
Copy link
Member

mattab commented Dec 18, 2015

Looks good to me! 👍

@mattab mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Jan 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants