Support json for system:config:set

[phil-davis]

Description

Add support for --type json to the occ config:system:set command.

See commands below for examples of the type of things that can be done.

This allows configuring multi-key system config options in a single command, avoiding catch-22's.

Related Issue

• Fixes OCC commands fail when configuring apps_paths #32511

Motivation and Context

In acceptance tests I want to be able to clear any apps_paths settings and then set up apps_paths in different ways for the tests. To do that I can get the testing app to run occ config:system:set but I need to be able to do it in a single occ command, to avoid the catch-22 in the referenced issue.

I noticed this challenge today when looking at how to get the appmanagement.feature acceptance tests to setup their environment via the testing app. (At present they "cheat" and directly call setSystemValue() in the backend - which will not work for proper blackbox testing).

How Has This Been Tested?

Manual occ commands like:

php occ config:system:set apps_paths --type=json --value='[{"path":"/home/phil/git/owncloud/core/apps","url":"apps","writable":1},{"path":"/home/phil/git/owncloud/core/apps2","url":"apps2","writable":1}]'

or

php occ config:system:set apps_paths 1 --type=json --value='{"path":"/home/phil/git/owncloud/core/apps2","url":"apps2","writable":1}'

and inspecting the result in config.php

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised: Document config:system:set --type=json owncloud-archive/documentation#4404
  (documentation needed)

Open tasks:

• Backport (if applicable set "backport-request" label and remove when the backport was done)

[patrickjahns]

json_decode will also return null when a value is empty

Please add a test covering the empty parts

[individual-it]

should be ==== or is there a reason why not?

[phil-davis]

I will fix.

[codecov]

Codecov Report

Merging #32512 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #32512      +/-   ##
============================================
+ Coverage     64.12%   64.12%   +<.01%     
- Complexity    18643    18645       +2     
============================================
  Files          1176     1176              
  Lines         70189    70195       +6     
  Branches       1270     1270              
============================================
+ Hits          45007    45013       +6     
  Misses        24812    24812              
  Partials        370      370

Flag	Coverage Δ	Complexity Δ
#javascript	52.89% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	65.4% <100%> (ø)	18645 <0> (+2)	⬆️

Impacted Files	Coverage Δ	Complexity Δ
core/Command/Config/System/SetConfig.php	100% <100%> (ø)	28 <0> (+2)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae984b3...68b532c. Read the comment docs.

[phil-davis]

Fixed up to have === null and added a test case for if someone provides an empty string as the value when type is json.
@patrickjahns @individual-it ready for review again.

[phil-davis]

This is an enhancement to the config:system:set command that everybody gets. So it needs agreement that this is "a good thing" to do, not just from QA-testing people.
Then I can make a documentation issue and PR also.

[patrickjahns]

Just thinking of the other way around - what should config:system:get return?
And will a export from config:system:get and pipe to config:system:set lead to the same configuration?
Does config:system:delete work properly with the values in the array?

( note I realize that these questions do not address the PR directly, but was wondering about the mechanisms in general )

[phil-davis]

config:system:delete gets rid of a whole entry, including anything in the tree under it. It works fine at sub-levels:

php occ config:system:delete apps_paths 1

worked for me to remove entry 1, and leave behind entry 0.

config:system:get has an --output json option:

php occ config:system:get --output json apps_paths
[{"path":"\/home\/phil\/git\/owncloud\/core\/apps","url":"apps","writable":1},{"path":"\/home\/phil\/git\/owncloud\/core\/apps2","url":"apps2","writable":1}]

and

php occ config:system:get --output json apps_paths 1
{"path":"\/home\/phil\/git\/owncloud\/core\/apps2","url":"apps2","writable":1}

That works out nicely. The output of those is the JSON that I fed to config:system:set

[patrickjahns]

LGTM - rest needs to be decided by other maintainers

[mmattel]

Then I can make a documentation issue and PR also.

👍

[phil-davis]

@PVince81 if you have a moment this afternoon/evening, can you comment if this seems a good idea?

If it is, then I would like to make use of it in some acceptance test code. So I am happy to have it merged. Then backport can be merged and I will write a doc issue and PR for 10.0.10

[PVince81]

nice addition 👍

[phil-davis]

Doc issue owncloud-archive/documentation#4404 raised.

[PVince81]

@phil-davis please backport

[phil-davis]

Backport stable10 #32524 already merged.