-
Notifications
You must be signed in to change notification settings - Fork 0
25895/get all config scopes #4
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
Conversation
a-leurs
commented
Jan 6, 2023
- #26061 Change format of output
- #25895 Add command for all config paths and subset of config paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good changes in general, just a couple of details.
$scopeValues = array(); | ||
foreach($all_configs as $element) { | ||
$result_values = $this->scopeHintService->getConfigValuesForScopes($element); | ||
foreach($result_values as $element2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a speaking name instead of $element2
. I cannot figure out what $element2
contains by just looking at this function.
src/Service/ScopeHintService.php
Outdated
|
||
function displayArrayRecursively($array, &$result_array, $path) : void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I try to avoid variables as references and use return values instead. Usually (including this case), it's more readable.
Also please add types for the input parameters.
src/Service/ScopeHintService.php
Outdated
return []; | ||
foreach ($array as $key => $value) { | ||
if (is_array($value)) { | ||
$this->displayArrayRecursively($value, $result_array, $path . $key . '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good usecase for recursive function calls. It's also good to have the "recursive" keyword in the function name 👍
src/Service/ScopeHintService.php
Outdated
@@ -104,6 +115,9 @@ public function getConfigValuesForScopes( | |||
} | |||
} | |||
|
|||
$configValues[0]["values"] = str_replace('\n', ' ', $configValues[0]["values"]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, we only use single quotation marks in PHP code. An exception would be "\n"
, which doesn't work if it is '\n'
. I am interested: does it work here?
So, I'd use single quotation marks where you use double quotation marks and vice versa in the code block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avstudnitz You're right. '\n'
doesn't do anything. It needs to be put in double quotes.
But I only did that because I thought the line break destroys the creation of the cells of the table.
But it doesn't. So the code can be removed.
Change variable names to Camel case, removed reference from recursive function, removed unnecessary code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better now, thanks.
There are still some formatting / code style issues like spaces and line breaks in the wrong position or missing. I'd suggest you configure code style checking with PHPMD and PHPCS and at least run it manually on the command line.
@avstudnitz Thanks. I will check the coding style with PHPMD and PHPCS later. Had to merge this PR because it was blocking me on another task. |