-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Config post processing #22
Config post processing #22
Conversation
adfe09f
to
2ae6957
Compare
One common issue when using third-party libraries is that the configuration keys will still reference ZF artifacts: namespaces, classes, and configuration keys. This patch provides a recursive configuration post-processor that rewrites those references using the rules used when performing migrations, ensuring that they now reference the Laminas equivalents. The post processor, `Laminas\ZendFrameworkBridge\ConfigPostProcessor` can be used with laminas-config-aggregator. Additionally, you can register `Laminas\ZendFrameworkBridge` as a module in MVC applications, and it will register a `ModuleEvent::EVENT_CONFIG_MERGE` listener that will pass merged configuration to the post processor, and pass the transformed configuration back to the `ConfigListener`. The module is NOT exposed via the `extra.laminas.module` directive, to ensure that it is opt-in, and thus can also be disabled. The migration tooling will be updated to register the module in MVC applications, and the post processor in Expressive applications. I also recommend that we have the migrations package depend on this one, so that all rewrite rules are in one package.
2ae6957
to
b6bb25a
Compare
Provides mocks for the various collaborators, and then tests interactions.
98bb5ee
to
6eec523
Compare
This patch imports the edge case tests from laminas-migration.
a6635fd
to
6f0ce79
Compare
Updates the logic in ConfigPostProcessor to overwrite values if they exist and are non-arrays, and only merge if they are both array values.
The various Laminas packages all define aliases of the legacy ZF classes to the Laminas counterparts. As such, if we were to rewrite those aliases, we end up with a cyclical alias situation (e.g., `'Zend\Db\Adapter\Adapter' => 'Laminas\Db\Adapter\Adapter'` would become `'Laminas\Db\Adapter\Adapter' => 'Laminas\Db\Adapter\Adapter'`). As such, this patch modifies the `rewriteValue` functionality to take into account the key being rewritten, and to skip rewriting for arrays under the 'aliases' key.
I've now tested this in a variety of full-application scenarios, and all worked perfectly following a migration. While it adds some initial overhead, that overhead is removed in production due to configuration caching. |
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.
@weierophinney Check out my comments, I think we can have couple edge cases we should also handle.
There may be third-party libraries that use "zend-expressive" as a configuration key prefix. To avoid rewriting those, this patch adds functionality for replacing exact matches, and then falls back to the general rewrite rules (which omit that match).
Ideally, we'd have a typehint here. But since it is documented as accepting strings only, anybody using it with a non-string value should likely receive a notice from PHP about it.
Prevents type munging.
This patch is actually a refactor of how replacements are made to use a rules engine. Each rule is run, and can return null or a callable. If a null is returned, it checks the next rule, and so-forth. If a callable is returned, it is used to perform the replacement. This approach allows us to have rules of different granularity, and for specific conditions. I rewrote the "exact replacements" functionality as a ruleset, and added two more: - one for when the key is "aliases" and it has an array value - one for a non-null key and an array value In the first case, it now invokes the new method `replaceDependencyAliases`, which replaces the targets for aliases, but not the keys. In the second case, it calls `__invoke()` on the value. The fallback checks for a string value, returning the result of `Replacements::replace()` on it if so, and the unchanged value otherwise. The `replaceValue()` method was renamed `replace`, and is now called both for keys (in which case the `$key` value passed to a ruleset is `null`) as well as values.
I've pushed changes to address the various areas of feedback. The code is already following the same merge algorithm as zend-config-aggregator, which means:
I've added tests to cover this as well, to make it explicit. I have also refactored the internals to use rulesets. Each ruleset is a callable that receives the value and key, and then either returns a null or a callable. If a callable is returned, that value is used to perform replacements on the value. If no ruleset matches, the default ruleset is to replace string values using the string translation rules, and otherwise return the value verbatim (which was the previous behavior). I have added the following rulesets:
This approach should allow us to add more rulesets as we need them, while keeping the code maintainable (no switch/case or if/elseif madness). |
Adds a test to ensure the precedence order when merging values. It should be: - If both values are arrays, they are merged, with the second taking precedence over the first. - If the original value is not an array, the second value overwrites the original. Additionally, this adds a check to see if the value provided to `merge()` is an array; if not, it will append the value to the original array.
No longer relevant if we are targetting PHP 5.6 as the lowest supported PHP version.
b9de226
to
5efbd42
Compare
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.
The code is already following the same merge algorithm as zend-config-aggregator, which means:
- If the first value is a scalar, the second value overwrites.
- If the first value is an array, and the second value is scalar, the second value is appended.
- If both values are arrays, they are merged, with the second taking precedence.
null
is not a scalar. What should happen if:
- the first value is null and 2nd is not
- the 2nd value is null and the first is not
In test, what I noted in comments, I am missing also cases:
- scalar => array
- null => array
In general changes looks good.
I am thinking only if replacing everything in the configuration is the way we should go.
On migration is a bit different story, because this is just a tool which help update application to Laminas.
The tool which is replacing whole configuration of 3rd part libraries is quite risky: for example we can have routing defined in 3rd part library which can be changed, but links can be used separately on frontend (in the same 3rd party library) and these will be not changed.
A bit safer solution will be to replace only:
- configuration keys - so first level keys (but using just part of the rules - just configuration keys)
- service manager/plugin managers configuration (I guess including custom plugin managers)
I think we shouldn't touch routing completely.
Also something what is not Laminas/Expressive/Apigility configuration on 1st level - so custom 3rd party configuration.
It looks like we might have an issue also with invokable
configuration in SM (this is array alias => target
or if the alias is not provided target is registered).
What we would need do here is:
- if alias is provided do not rewrite it
- otherwise (alias is not provided) - add class alias as the target is, and rewrite the target.
I'll have another look in the morning. For now it looks that in most of cases it's gonna work fine. By rewriting everything (recursion, without checking what was the 1st level key) it is similar to approach we take first with migration script.
/** @var string[] */ | ||
private $replacements; | ||
|
||
public function __construct(array $additionalReplacements = []) |
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.
we are not using this parameter now, so not sure if we should keep it
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.
I'd like to, as it provides an extension point if we want to expand the replacement list.
I disagree, and specifically because of Apigility. Apigility defines a number of routes, and the top-level name of the route is "zf-apigility". If we do not touch that, we potentially have some issues if folks are adding items to it (in particular, documentation paths). But this could potentially be considered a special case, and we could alter only that one. |
As an exact replacement, as the zfcampus/zf-apigility package defines the top-level key.
Removes verbiage around `$preserveNumericKeys`, which is no longer relevant.
Instead of calling `$this->replace($value, $newKey)` in multiple locations, call it once, and re-use it withing conditionals.
That's correct, I've checked it and for example: But the problem with this is that when you build the route you need provide the key to, so if 3rd-party library adds some route it will be unusable in the views because we are not rewriting views. So in views it will be still something like It looks like trying to add Laminas compatibility for everything is going to be impossible. |
Adds test cases for: - Merging null with scalar (scalar overwrites) - Merging scalar with null (does not overwrite) - Merging array with null (no change to array) - Merging scalar with array (array overwrites) - Merging two scalars (second overwrites first) Due to these tests, I identified code changes necessary to make the post processor behave per the expectations.
Use isset instead of array_key_exists for check.
9b104cf
to
e10de60
Compare
I never expected we'd get 100% compatibility! The fact that we're as far as we are is mind-blowing, as this configuration post processing piece answers essentially all but one piece of open feedback! With regards to the routing bit: I think you're right, in the end. The only routes we actually ship that are problematic are the ones in Apigility — and there's very little chance that folks are extending those routes, as they're specific to the admin UI. Even the documentation one is unlikely to be an issue, as those are handled by our own modules; if they're handled by somebody else's, they're likely using their own routing table. If they are not, that becomes an FAQ item. My current thinking at this point is:
This would be a trivial change to what I'm already doing, and I think would alleviate most of your own concerns. |
Since URL generation is dependent on the route names, we should not rewrite them.
This patch does two things. First, it modifies how replacements are matched and triggered, passing a hierarchical array of keys so that rulesets can determine where they are in the hierarchy, as well as access the keys present. This allows us to, for instance, know when we are matching a top-level key. Second, it adds a ruleset for rewriting invokables configuration. If the alias does not change but the target does, then that change is made. If the alias changes, too, then: - An entry is added for the original alias, pointing to the rewritten target. - An entry is added for the new alias, pointing to the rewritten target. This ensures aliases work as expected, and the original service names are still present in configuration.
We want to keep original top-level keys and contents so that developers can compare the two, and so that any factories that depend on the old keys will continue to work.
Updated: I updated the content to incorporate the changes introduced in a22283c @michalbundyra Okay, changes are all created and pushed! Summary of changes since last review:
I think at this point, we should likely merge and tag as soon as we can, so we can get this feature in the hands of users; we can then add more tests, scenarios, and rulesets as we uncover any issues. |
Invokables are essentially an alias map. This patch modifies the code to remove the separate logic for rewriting invokables, and instead have the rewrite rule for aliases also handle invokables. That way, the rule becomes: rewrite the target only. This ensures the original service name persists, but now maps to the new target class.
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.
@weierophinney It looks fine now, just some very minor comments.
src/ConfigPostProcessor.php
Outdated
* nested configuration. | ||
* @return array | ||
*/ | ||
public function __invoke(array $config, $keys = []) |
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.
we can have typehint on $keys
- array
src/ConfigPostProcessor.php
Outdated
* @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com) | ||
* | ||
* @param array $a | ||
* @param array $b |
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.
minor: these are redundant as we have typehints on params
src/ConfigPostProcessor.php
Outdated
* This same logic can be used for invokables, which are essentially just | ||
* an alias map. | ||
* | ||
* @param array $value |
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.
invalid - as we do not have this param anymore in this method
* | ||
* @param ModuleManager $moduleManager | ||
*/ | ||
public function init($moduleManager) |
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.
I believe we can have typehint here?
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.
I deliberately chose not to so I could test without having a dependency on a package that does not yet exist.
I'll add it once we have transferred the packages to their new homes.
src/Module.php
Outdated
* Type-hinting deliberately omitted to allow unit testing | ||
* without dependencies on packages that do not exist yet. | ||
* | ||
* @param ModuleEvent $moduleEvent |
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.
invalid param name - inconsistent with signature. Can't we just have typehint on param?
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.
I'll update the annotation; see my comment about the init()
method for why I don't have an actual typehint here.
class ConfigPostProcessorTest extends TestCase | ||
{ | ||
/** | ||
* @return iterable |
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.
I am not sure if it is right type, always thought it should be Generator for yield
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.
iterable
works for any value that can be iterated. That includes generators, iterators, and arrays. I chose iterable
so that we can refactor this to use an array later if we want.
- Removes unnecessary parameter annotations. - Adds typehints where needed and useful. - Fixes annotations where they were incorrect.
Creates a changelog for version 0.3.3, adds changelog entries for #22, and sets the release date.
One common issue when using third-party libraries is that the configuration keys will still reference ZF artifacts: namespaces, classes, and configuration keys.
This patch provides a recursive configuration post-processor that rewrites those references using the rules used when performing migrations, ensuring that they now reference the Laminas equivalents.
The post processor,
Laminas\ZendFrameworkBridge\ConfigPostProcessor
can be used with laminas-config-aggregator. Additionally, you can registerLaminas\ZendFrameworkBridge
as a module in MVC applications, and it will register aModuleEvent::EVENT_CONFIG_MERGE
listener that will pass merged configuration to the post processor, and pass the transformed configuration back to theConfigListener
.The module is also exposed via the
extra.laminas.module
directive, allowing end-users to opt-in to using the post processor in their MVC applications.The migration tooling will be updated to register the module in MVC applications, and the post processor in Expressive applications.
This patch also introduces
Laminas\ZendFrameworkBridge\Replacements
, and it contains the default list of replacements, as pulled from the laminas-migration library. I suggest that we update laminas-migration to depend on this library, so that the replacements are in a single location.Finally, I've updated the plugin to only support PHP 5.6 and up, for the same reasons the migration tooling and dependency plugin only support those versions and up.
TODO