Sorted arrays #33
Sorted arrays #33
Conversation
The configuration files that are generated can be extremely big. This is the case in Apigility for example. By sorting the config by key the files are more readable and one can expect where to find data.
Sorting can now be disabled based on the setSortingEnabled method.
There was a problem hiding this comment.
This looks like a great new feature, @waltertamboer! I've noted a few changes, primarily around documentation.
CHANGELOG.md
Outdated
| - Nothing. | ||
| - The configuration that is written is now sorted by its keys. | ||
| This new feature is enabled by default. Sorting can be disabled by | ||
| calling `->setSortingEnabled(false);` |
There was a problem hiding this comment.
Where would you call this, exactly?
Generally speaking, this module is used in development by zf-apigility-admin. As such, you would need to call it on the instance itself, which means using a custom factory, or a delegator factory. You should provide examples of that both here in the changelog, as well as the README.md file.
There was a problem hiding this comment.
Also, this should note that this feature is available since 1.4.0.
There was a problem hiding this comment.
Regarding the version: Isn't it clear that it's part of 1.4.0 since it's under the 1.4.0 - TBD header?
Regarding documentation, is it really up to to this module to dictate how this method should be used? For all we know someone could install the package via composer and just use ConfigResource class without the service manager stuff. It's no problem for me to add examples but it feels a bit out of scope. I do agree that the current description is unclear. I've changed the last line to "Sorting can be disabled by calling setSortingEnabled(false); on the ConfigResource class.".
If you feel really strong about adding examples, I'll add them to the README, no problem.
src/ConfigResource.php
Outdated
| */ | ||
| public function setSortingEnabled($enabled) | ||
| { | ||
| $this->sortingEnabled = $enabled; |
There was a problem hiding this comment.
Cast this value to (bool) when assigning.
There was a problem hiding this comment.
Yes, that's better, changed. Type hinting would be even better of course ;)
src/ConfigResource.php
Outdated
| @@ -349,4 +353,15 @@ protected function invalidateCache($filename) | |||
|
|
|||
| opcache_invalidate($filename, true); | |||
| } | |||
|
|
|||
| protected function sortKeysRecursively(array &$data) | |||
There was a problem hiding this comment.
Please add a docblock for this new method.
There was a problem hiding this comment.
Done, I've also removed the return statement since it didn't make sense.
test/ConfigResourceTest.php
Outdated
| @@ -402,4 +402,80 @@ public function testDeleteNonexistentKeyShouldDoNothing() | |||
| $test = include $this->file; | |||
| $this->assertEquals($expected, $test); | |||
| } | |||
|
|
|||
| public function testDataIsSorted() | |||
There was a problem hiding this comment.
Rename this to testDataIsSortedByKey().
test/ConfigResourceTest.php
Outdated
| self::assertEquals(1, next($test)); | ||
| } | ||
|
|
||
| public function testDataIsSortedRecursively() |
There was a problem hiding this comment.
Rename this to testDataIsSortedRecursivelyByKey().
test/ConfigResourceTest.php
Outdated
| self::assertEquals('2', $secondVal); | ||
| } | ||
|
|
||
| public function testDataIsNotSortedWhenDisabled() |
There was a problem hiding this comment.
Rename this to testDataIsNotSortedByKeyWhenSortingIsDisabled().
| @@ -92,6 +92,9 @@ service serves the purpose of providing the necessary dependencies for `ConfigRe | |||
| methods such as `patch()` and `replace()`. The service returned by the service manager is bound to | |||
| the file specified in the `config_file` key. | |||
|
|
|||
| The config that is written is sorted by its keys. To disable this, use`setSortingEnabled` and provide | |||
| `false` as a parameter. | |||
There was a problem hiding this comment.
There's still a problem here: where does the user call this method, exactly? Particularly as this is primarily an implementation detail for the admin API/UI. In scanning zf-apigility-admin, we generally create ConfigResource instances within factories for the various models; as such, this would require providing alternatives for each and every one of these factories. I'm beginning to think we may need a configuration switch that any factory that creates a ConfigResource would look at when creating an instance. For BC purposes, this would be disabled by default (to ensure that a minimal set of changes is made to existing configuration files), and users would opt-in to the new behavior.
Thoughts?
There was a problem hiding this comment.
Ah I see what you mean now. Yes this does sound like a challenge. As far as I understand the changes made here are ok. The question is just how to properly use it in Apigility.
I'm not familiar enough with the architecture behind this but could it be as simple as reducing all the places where an instance of ConfigResource is created, to just 1 place by introducing a new class that handles the writing of config resources? Either way this feels like a BC break.
Thinking out of the box. Maybe setSortingEnabled should not even exist, or maybe it's not the responsibility of a ConfigResource to toggle sorting.
|
This repository has been closed and moved to laminas-api-tools/api-tools-configuration; a new issue has been opened at laminas-api-tools/api-tools-configuration#1. |
|
This repository has been moved to laminas-api-tools/api-tools-configuration. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:
|
This is a new feature where the configuration written is sorted by its keys. This is useful in case of large configs so developers can expect where data is located in the file.
This new feature is enabled by default and can be turned off by calling
->setSortingEnabled(false);.