FEATURE: Command line can parse associative array arguments

[gjwnc]

What I did

The command line RequestBuilder can now parse arguments for associative arrays like

./flow my.pckg:index --dimension.language=en --dimension.country de --dimension.\"key.with.a.dot\" mindblown

The result is, that the command line controller like

public function indexCommand(array $dimension){

will receive an associative array as described with a dot syntax in the cli arguments.

I also tried to beautify the code a bit.

[mhsdesign]

Not to be harsh - but i think the whole cli argument parsing could use a bit of work - can we not simply use a 3rd party lib instead of building it from scratch? I know some places symphony cli is used already and abstracted away by flow ...

[gjwnc]

:shrug: You mean something like that? I'm not sure how to handle associative arguments with that, but hadn't an in depth look yet, and this is also currently not available in flow.

I assume this decision needs to be discussed in the core team. Shall we close my PR?

From my point of view, the feature itself would be quite interesting because you could create/modify node dimensions or properties from the commandline or override settings or just implement more generic features on your own to test stuff.

How do we proceed with this PR and the refactoring in general?

[kitsunet]

Don't think we need to conflate the two, this seems like a useful addition, refactoring I would agree is necessary but can happen separately / in parallel.

[mhsdesign]

Hmm you did a nice job at commenting the functions and params - would be a shame to lose this :P

I just wanted to trigger the discussion for the bigger picture ... before its to late.

[gjwnc]

@grebaldi Sorry for my late response.

1. It neither reads nor writes very easily

I get that this seems cumbersome and I aggree with you on that point.

Before making a decision let's try to have a look at quoted example:

--dimension.\"key.with.a.dot\"

Tbh, I don't know exactly why the \ is needed, but if you look at the following demo script argtest.php:

<?php
var_dump($argv);

and run php ./argtest.php --dimension."key.with.a.dot" you get

array(2) {
  [0]=>
  string(13) "./argtest.php"
  [1]=>
  string(26) "--dimension.key.with.a.dot"
}

Notice, the quotes have been removed. I don't know what is swalling the " characters here, but I assume it's the bash - or whatever shell you're using. Maybe php itself. That is the reason why I used the escape character in the example, because php ./argtest.php --dimension.\"key.with.a.dot\" produces the correct string for processing in php:

array(2) {
  [0]=>
  string(13) "./argtest.php"
  [1]=>
  string(28) "--dimension."key.with.a.dot""
}

2. Depending on the execution environment, string escapes may pose a problem. If you'd use the `flow` command via DDEV (see: https://ddev.com/):

ddev flow --dimension.\"key.with.a.dot\"

then the escapes would be swallowed, which breaks the parameter.

What about something like ddev flow --dimension.\\"key.with.a.dot\\", or maybe even using \\\"? I needed to use the \\\" syntax in some of my CI/CD scripts, because there are so many layers which process the scripts until it finally reaches the target execution environment.
Don't get me wrong here, I don't argue against it, but I want to clear up some technical details beforehand.

Regarding your alternative syntax --dimension[key.with.a.dot]. This is something like JSONPath, which would be an established standard.
There is also YAMLPath but I think this is more like a proposal, but already pretty evolved, I think.

Generally speaking, it would be nice to be guided by a standard. I used the quote syntax version from the yamlpath because I was more thinking about overriding Settings.yaml with command arguments - thus more related to yaml.

The YAMLPath docs mention the following syntax:

• Hash sub-keys: hash.child.key or /hash/child/key
• Demarcation for dotted Hash keys: hash.'dotted.child.key' or hash."dotted.child.key" (not necessary when using forward-slash notation, /hash/dotted.child.key)

This would mean, that we implement both options " or ' and /, which I think would not be that much of a problem and also solves the issue with the swallowed characters.
On the other hand, the jsonpath syntax --dimension[key.with.a.dot] is now just a quick-fix with your code comments.

I would suggest, that we get other opinions on this, since our decision will stick with FLOW for quite some time.

@mhsdesign @kitsunet What do you think? How do we agree on a syntax? I'm fine with both but need to know which should be implemented. Maybe ask someone else for further opinions?

[kitsunet]

If I needed something this complex so far I opted for going with either providing a YAML/JSON file path and parsing that or providing a JSON string and parsing that. I have made a little proof of concept using the symfony console component to replace our parsing, but that is definitely stricter than what we have now. A feature like this here won't work out with that and I really would like to avoid supporting it long term.

[mhsdesign]

I just discussed something similar in another palace here: https://github.com/neos/flow-development-collection/pull/1903#issuecomment-1427457030 and i do like the syntax --dimension[key.with.a.dot]

Either way we should stay consistent in the codebase and DX.

[gjwnc]

This discussion came up, because I parsed the dot syntax on my own using preg_match_all, and I didn't think about alternatives like using Arrays. Because of the similar discussion, #1903 , I will refactor my changes and replace parsing via preg_match_all by using Arrays::setValueByPath. I already have a working example.

This basically moves the problem of the syntax to a central piece of code in the Arrays class. @mhsdesign Can you please make sure, that the changes discussed in #1903 will also be added to ::setValueByPath?

[mhsdesign]

If I needed something this complex so far I opted for going with either providing a YAML/JSON file path and parsing that or providing a JSON string and parsing that. I have made a little proof of concept using the symfony console component to replace our parsing, but that is definitely stricter than what we have now. A feature like this here won't work out with that and I really would like to avoid supporting it long term.

@gjwnc after discussing this with christian, i think it is the right way to move to symfony console where we can be glad to have one responsibility less when maintaining the code. Sadly that would also mean that we wouldn't be able to accept your efforts on this feature (still thank you for your spirit!), as symphony console doesn't support this feature and we would be able to switch then. The real problem is that our current parsing architecture is just "a bit" unreadable and hacky so it would be a much greater gain to use symphony console than to keep our current code and add yet another feature on top of it (which will make the code even more complex). Im sorry for not expressing my concerns more clearly earlier (i tried in https://github.com/neos/flow-development-collection/pull/2834#issuecomment-1183663963 ^^) I would certainly rethink my decision of this feature if we (someone) decides to rewrite the cli parsing and build a proper parser for it.

[gjwnc]

Argh, Bummer!
Since I don't want to put this into the trash bin completely, I'll try to move the code to a separate package and use aspects to provide the feature. It was bit of an effort and learning how the CLI processing works and I would like to keep this somewhere.

@mhsdesign Regarding symfony console: Tbh, I don't know much about it, so maybe that's also the reason I didn't grasp the range of your concerns. The docs mention an InputOption::VALUE_IS_ARRAY :shrug: Sooo, maybe this could be added in the future?
I don't want to start an argument here, since the decision is made. Maybe one could create a separate issue for refactoring the CLI and line out what is this all about and what needs to be changed? Are we talking about the next big refactoring for you, after the fusion parser :laughing: or is this an easy one :thinking: @kitsunet also mentioned this in other issues, see.
We can also discuss this further at NEOS con :relaxed:

You also mentioned #1903 which tries to update the syntax used by Arrays::getValueByPath. For completeness sake, do you think the PR from #1903 should also be added to Arrays::setValueByPath and Arrays::unsetValueByPath? There is quite a big discussion about the parsing there and I don't have an overview about it now, but the PR seems quite small.

[mhsdesign]

concerning Arrays::getValueByPath and friends, my current goal is to implement the dot escaping only for the configuration:show command (separately from the whole helper) and if we later feel like using this also in the helper (eg. every place where this method is somehow involved (a bit risky/ breaky) i think we can expand the syntax to the array utilities. Maybe 9.0 ^^