Implement Configuration\Builder in FrameworkExtension #598
Conversation
…rging and normalization This fixes some BC problems introduced in f9138d3. Some top-level can now be simply enabled by providing true/null in PHP/YAML. Additionally, the Configuration\Builder allows options to be unset by providing "false" (helpful for overriding activation in a previous config file). All options supporting these behaviors can be found in the Configuration.php file (look for canBeUnset() and treatNull/TrueLike()). Major changes: * Removed "enabled" option for profiler config. Profiler is now enabled if its config is true, null or a map. * Restore original config structure for validation namespaces. In PHP/YAML, namespaces are defined under annotations as an alternative to false (disabled) and true/null (enabled). For XML, annotation remains a boolean attribute for validation and a one or more optional namespace tags may appear within <app:validation />. During config normalization, namespace tags under validation will be moved to annotations to conform to the PHP/YAML structure (this occurs transparently to the user). * Restore behavior for router/templating config sections being optional (as shown in changes to session/validation test fixtures). If either top-level section is unset in the configuration, neither feature will be enabled and the user will no longer receive exceptions due to missing a resource option (router) or engines (templating). Resource/engines will still be properly required if the respective feature is enabled. * Remove unused router type option from XML config XSD. Type is only relevant for import statements, so this option is likely useless. Additional small changes: * Added isset()'s, since config options may be unset * Wrap registerXxxConfiguration() calls in isset() checks * Load translation.xml in configLoad(), since it's always required * Default cache_warmer value (!kernel.debug) is determined via Configuration class Things to be fixed: * Configuration\Builder doesn't seem to respect isRequired() and requiresAtLeastOneElement() (or I haven't set it properly); this should replace the need for FrameworkExtension to throw exceptions for bad router/templating configs * The config nodes for session options don't have the "pdo." prefix, as dots are not allowed in node names. To preserve BC for now, the "pdo." prefix is still allowed (and mandated by XSD) in configuration files. In the future, we may just want to do away with the "pdo." prefix. * Translator has an "enabled" option. If there's no use case for setting "fallback" independently (when "enabled" is false), perhaps "enabled" should be removed entirely and translator should function like profiler currently does. * Profiler matcher merging might need to be adjusted so multiple configs simply overwrite matcher instead of merging its array keys.
…igurations
The merging is done in three steps:
1. Normalization:
=================
All passed config arrays will be transformed into the same structure
regardless of what format they come from.
2. Merging:
===========
This is the step when the actual merging is performed. Starting at the root
the configs will be passed along the tree until a node has no children, or
the merging of sub-paths of the current node has been specifically disabled.
Left-Side Right-Side Merge Result
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-nothing- array Right-Side will be taken.
scalar scalar Right-Side will be taken.
array false Right-Side will be taken if ->canBeUnset()
was called on the array node.
false array Right-Side will be taken.
array array Each value in the array will be passed to
the specific child node, or the prototype
node (whatever is present).
3. Finalization:
================
The normalized, and merged config will be passed through the config tree to
perform final validation on the submitted values, and set default values
where this has been requested.
You can influence this process in various ways, here is a list with some examples.
All of these methods must be called on the node on which they should be applied.
* isRequired(): Node must be present in at least one config file.
* requiresAtLeastOneElement(): PrototypeNode must have at least one element.
* treatNullLike($value): Replaces null with $value during normalization.
* treatTrueLike($value): Same as above just for true
* treatFalseLike($value): Same as above just for false
* defaultValue($value): Sets a default value for this node (only for scalars)
* addDefaultsIfNotSet(): Whether to add default values of an array which has not
been defined in any configuration file.
* disallowNewKeysInSubsequentConfigs(): All keys for this array must be defined
in one configuration file, subsequent
configurations may only overwrite these.
* fixXmlConfig($key, $plural = null): Transforms XML config into same structure
as YAML, and PHP configurations.
* useAttributeAsKey($name): Defines which XML attribute to use as array key.
* cannotBeOverwritten(): Declares a certain sub-path as non-overwritable. All
configuration for this path must be defined in the same
configuration file.
* cannotBeEmpty(): If value is set, it must be non-empty.
* canBeUnset(): If array values should be unset if false is specified.
Architecture:
=============
The configuration consists basically out of two different sets of classes.
1. Builder classes: These classes provide the fluent interface and
are used to construct the config tree.
2. Node classes: These classes contain the actual logic for normalization,
merging, and finalizing configurations.
After you have added all the metadata to your builders, the call to
->buildTree() will convert this metadata to actual node classes. Most of the
time, you will not have to interact with the config nodes directly, but will
delegate this to the Processor class which will call the respective methods
on the config node classes.
|
this looks much better. the extension is way more readable, the configuration stuff is a bit odd at first, but the API is fairly self explanatory and its not like one reads this every day. we might even consider writing some auto documentation generation tool for this? something unrelated i just stumbled over in the code: not sure if its good that we are hardcoding the relative paths between Bundles and Components. |
|
I also prefer Boolean Regarding the |
|
+1 for empty() i think we can still make people accustomed to using option: true instead :) for the file, yeah i wasnt implying its your fault. and yes the file definitely needs to be in the component. but i think i would prefer to make this configurable with a sensible default. |
In order for this to work the entire path leading up to the required node, must be set to required. If not, this will only be validated if the parent is present in the configuration. In that case, if a router node is present it is required to have a resource node.
I've added a new option ->performNoDeepMerging() for these cases. So, if a node where this has been set is overwritten, the entire sub-tree is simply taken from the right-side without passing it to the child nodes. |
There was a problem hiding this comment.
Since this is the default behavior, you could omit the calls to ->treatNull/True here.
There was a problem hiding this comment.
Ah, that explains some other behavior I was seeing in my unit tests :)
|
I like the approach better than the previous one. +1 |
|
I like it - nice work everyone. The most complex parts read better than their equivalent solution before this. I still feel strongly, however, that the default values should be stored in yaml and loaded in (so that the user has an example config). I realize this would look awkward for the prototype entries, but I feel like it's something we should continue to shoot for. To be pragmatic, the yaml file could specify the easy defaults and leave the harder ones inside the class. We could still include commented-out examples of the more difficult defaults (e.g. example entries matching the prototype defaults). |
|
I'm not sure this is worthwhile as far as security is concerned, only a few nodes actually have default values, the others are provided dynamically at runtime by different factories, so we couldn't even have a central yaml file with all of them. Do you think it's really necessary, or couldn't that be solved by providing good documentation like the configuration reference for security? |
|
Ryan: I don't think that's feasible for FrameworkExtension, as the only constant default is the session's storage_id ("native"), and that default only comes into play if the top-level session option is set. If we moved that into some default YAML file, that top-level option would then exist all the time and inadvertently enable sessions. Can the benefit of providing example configs be realized through test fixtures and, as Johannes pointed out, additional documentation? |
|
Hmm, ok - you both know about the challenges better than I do. My symfony1 workflow is usually to copy the config from a plugin and paste it into my application to be customized. We can accomplish that in other ways - via docs and/or a real config example file (which could be doubled as a test fixture as jeremy said). It's less ideal, but that's probably ok and could always be revisited later if this does in fact become a problem. |
|
When I apply the patch, I get this error message with the sandbox: |
|
Johannes fixed the bugs. |
|
To be fair, it was a bug in my code rather than his ;) |
5 participants
Johannes' latest progress doesn't seem to be in fabpot/master, so my branch includes commit(s) from his config branch (which is being rebased regularly). I'm not sure how much of what I need is already in fabpot/master, but I do know that
Symfony\Component\DependencyInjection\Configuration\Processoris currently missing. This pull request should remain in-progress at least until his revised component classes can be pulled and I can rebase to remove Johannes' commits from my own branch.I can't successfully complete all unit tests due to a failure with
DependencyInjection\Configuration\MergeTest, but I have things working within FrameworkBundle:This should be enough to start reviewing the work. All in all, implementing the config builder helped eliminate some of the annoying BC breaks from my last configuration merge implementation. My take on the config class is that, while it certainly appeared unwieldy at first glance, it was ultimately very helpful and a joy to work with. IMO, the Configuration class is quite readable as a result.
There is room for improvement (I listed some issues in my commit message), but I believe what we have here is functional and a step above the prior refactoring.