Implemented FirewallPluginInterface #31
Conversation
There was a problem hiding this comment.
Tests fail. Other than that looks good, thank you!
I wonder if it would make sense to implement this as a general solution as we don't know if there are other such nodes?
To be honest, I can't think of one at the moment and we should not overcomplicate things right now as we're still in the early stages. I think the firewall is the 99% case and if there will be another one we might still build a general solution. Even if we build the general solution I still think it is a good thing to keep the FirewallPluginInterface because it is more specific in naming and pretty obvious why it's there. It might still map to a new, more general solution, in the future so we don't block anything here imho :)
src/ContaoManager/Plugin.php
Outdated
| { | ||
| return array_merge( | ||
| $config, | ||
| Yaml::parse(file_get_contents(__DIR__.'/../Resources/contao-manager/security_firewalls.yml')) |
There was a problem hiding this comment.
Why are you not passing the loader as argument so we can do
$loader->load('@ContaoManagerBundle/Resources/contao-manager/security_firewalls.yml');?
There was a problem hiding this comment.
It has to be an array so you can order it. That's the whole point about this plugin :)
|
I have rewritten the implementation to use a more generic approach. The new This has several advantages:
The tests are currently failing because of #29 |
| $container = new PluginContainerBuilder($this->getPluginLoader(), []); | ||
| $container->getParameterBag()->add($this->getKernelParameters()); | ||
|
|
||
| if (class_exists('ProxyManager\Configuration') && class_exists('Symfony\Bridge\ProxyManager\LazyProxy\Instantiator\RuntimeInstantiator')) { |
There was a problem hiding this comment.
You could use Configuration::class and RuntimeInstantiator::class here.
There was a problem hiding this comment.
That code is a direct copy from Symfony so I'd rather not change it…?
see #24
This solves the issue that security firewall configuration cannot be extended (
cannotBeOverwrittenis set on theTreeBuilder). I wonder if it would make sense to implement this as a general solution as we don't know if there are other such nodes?