Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Sorted arrays #33

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ All notable changes to this project will be documented in this file, in reverse

### Added

- 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);` on the `ConfigResource` class.

### Changed

Expand Down
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.


#### ZF\Configuration\ConfigResourceFactory

`ZF\Configuration\ConfigResourceFactory` is a factory service that provides consumers with the
Expand Down
63 changes: 42 additions & 21 deletions src/ConfigResource.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ class ConfigResource
*/
protected $opcacheEnabled = false;

/**
* Whether or not sorting of the config is enabled.
*
* @var bool
*/
protected $sortingEnabled = true;

/**
* @var ConfigWriter
*/
Expand All @@ -51,6 +58,16 @@ public function __construct(array $config, $fileName, ConfigWriter $writer)
$this->writer = $writer;
}

/**
* Enables or disables the sorting of the config data.
*
* @param bool $enabled
*/
public function setSortingEnabled($enabled)
{
$this->sortingEnabled = (bool)$enabled;
}

/**
* Allow patching one or more key/value pairs
*
Expand Down Expand Up @@ -90,12 +107,7 @@ public function patch($data, $tree = false)
}
$localConfig = ArrayUtils::merge($localConfig, $patchValues);

// Write to configuration file
$this->writer->toFile($this->fileName, $localConfig);
$this->invalidateCache($this->fileName);

// Reseed configuration
$this->config = $localConfig;
$this->overWrite($localConfig);

// Return written values
return $data;
Expand All @@ -120,15 +132,7 @@ public function patchKey($key, $value)
}
$config = $this->replaceKey($key, $value, $config);

// Write to configuration file
$this->writer->toFile($this->fileName, $config);
$this->invalidateCache($this->fileName);

// Reseed configuration
$this->config = $config;

// Return written values
return $config;
return $this->overWrite($config);
}

/**
Expand All @@ -141,7 +145,12 @@ public function patchKey($key, $value)
*/
public function overWrite(array $data)
{
if ($this->sortingEnabled) {
$this->sortKeysRecursively($data);
}

$this->writer->toFile($this->fileName, $data);

$this->invalidateCache($this->fileName);

// Reseed configuration
Expand Down Expand Up @@ -240,13 +249,8 @@ public function deleteKey($keys)
}

$this->deleteByKey($config, $keys);
$this->writer->toFile($this->fileName, $config);
$this->invalidateCache($this->fileName);

// Reseed configuration
$this->config = $config;

return $config;
return $this->overWrite($config);
}

/**
Expand Down Expand Up @@ -349,4 +353,21 @@ protected function invalidateCache($filename)

opcache_invalidate($filename, true);
}

/**
* Sorts the provided array recursively based on its keys.
*
* @param array $data The array of data to sort.
* @return void
*/
protected function sortKeysRecursively(array &$data)
{
foreach ($data as &$value) {
if (is_array($value)) {
$this->sortKeysRecursively($value);
}
}

ksort($data);
}
}
76 changes: 76 additions & 0 deletions test/ConfigResourceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -402,4 +402,80 @@ public function testDeleteNonexistentKeyShouldDoNothing()
$test = include $this->file;
$this->assertEquals($expected, $test);
}

public function testDataIsSortedByKey()
{
// Arrange
$config = [
'z' => 1,
'a' => 2,
];

$configResource = new ConfigResource($config, $this->file, new PhpArray());

// Act
$configResource->overWrite($config);

// Assert
$test = include $this->file;

self::assertEquals(2, current($test));
self::assertEquals(1, next($test));
}

public function testDataIsSortedRecursivelyByKey()
{
// Arrange
$config = [
'z' => [
'z' => 1,
'a' => 2,
],
'a' => [
'z' => 1,
'a' => 2,
],
];

$configResource = new ConfigResource($config, $this->file, new PhpArray());

// Act
$configResource->overWrite($config);

// Assert
$test = include $this->file;

$firstKey = key($test);
$firstVal = current($test[$firstKey]);
self::assertEquals('a', $firstKey);
self::assertEquals('2', $firstVal);

next($test);

$secondKey = key($test);
$secondVal = current($test[$secondKey]);
self::assertEquals('z', $secondKey);
self::assertEquals('2', $secondVal);
}

public function testDataIsNotSortedByKeyWhenSortingIsDisabled()
{
// Arrange
$config = [
'z' => 1,
'a' => 2,
];

$configResource = new ConfigResource($config, $this->file, new PhpArray());

// Act
$configResource->setSortingEnabled(false);
$configResource->overWrite($config);

// Assert
$test = include $this->file;

self::assertEquals(1, current($test));
self::assertEquals(2, next($test));
}
}