Introduces Config Providers #110

gsteel · 2022-01-11T13:20:25Z

Q	A
BC Break	possibly
New Feature	yes
RFC	yes
QA	yes

Description

Moves all inline factory and alias configuration to config providers improving visibility of default settings and whilst preserving current behaviour.

The potential BC break is that any class extending the existing plugin managers might have property defaults for $factories and $aliases overwritten when parent::__construct is called - this could be mitigated by declaring a public constant for the default aliases and factories on the config providers, or maybe adding a protected property in order to set the defaults conditionally. Maybe we could mark the plugin managers as final (With an annotation) here too.

The purpose of adding the config provider now, is that I'm hoping we can, for 3.0, implement factories for the view helpers, so that we can inject helper dependencies in constructors instead of reaching into the setter injected view and retrieving plugins there. I reckon that this will make testing easier in future, make the process of overriding view helpers more explicit and ease the path to removing as much state as possible from the helpers themselves.

Another reason for the config providers is so that we can introduce factories for the plugin managers themselves enabling packages like mvc, mezzio and any others to provide delegators for any setup specific to those packages. In turn, this might make it easier to decouple from MVC… I know that the config providers don't enable setting up factories in themselves but they do provide a central point of reference for consumers trying to figure out how everything is wired up.

Sorry… I'm rambling now…

…sibility of default settings Signed-off-by: George Steel <george@net-glue.co.uk>

Signed-off-by: George Steel <george@net-glue.co.uk>

…endencies` as the plugin managers cannot work without it. Signed-off-by: George Steel <george@net-glue.co.uk>

…dependencies because they are both referenced directly in the code base Signed-off-by: George Steel <george@net-glue.co.uk>

…ker doesn't complain about them Signed-off-by: George Steel <george@net-glue.co.uk>

Signed-off-by: George Steel <george@net-glue.co.uk>

gsteel · 2022-02-09T15:32:41Z

@Ocramius, @froschdesign and @weierophinney - This is likely going to make the merge-up into 3.0 pretty bad, also, separating out all of the config into config providers, whilst making things tidy might be better off going into 3.0 where the change can be documented properly - also removes any ambiguity as to whether this is a BC break or not.

Please feel free to close if 3.0 would be a better target, then 2.20 could go out the door maybe, then I can stop sending in pulls for 2.x 😬

Ocramius · 2022-02-09T15:36:59Z

I'll be honest: I haven't reviewed this patch at architectural level, only glanced at it.

his is likely going to make the merge-up into 3.0 pretty bad

How bad? :D It's not that many changes - conflict resolution doesn't seem too complex.

gsteel · 2022-02-09T16:00:24Z

Merge up wont be terrible I guess… Lock file, anything with Zend in it will need to be removed for 3.0 as that's already been done. Identity helper factory is also Zend related. No biggie…

The main point of this is so that in mezzio/laminaszendviewrenderer we can update composer to declare extra.laminas.component or whatever it is, so that the config provider is supplied automatically by way of component installer and also the equivalent thing for MVC perhaps with its modules.

The only BC I'm concerned about here is if the plugin managers have been extended by users, specifically the constructor, then this pull might blow that up.

boesing

Overall, nice addition.

Please keep in mind that messing around with factories and aliases might introduce BC breaks:

laminas/laminas-mvc#99

If you can confirm that the factories and the aliases are 100% the same as in the previous version, we're fine here.

What actually does introduce a BC break is that plugin manager which overrides factories and aliases after parent::__construct(); call. This should be fixed.

On top of that, adding support for laminas-component-installer is mandatory here and then, you will see, that it is not possible to have more than one ConfigProvider per component.

So please merge all ConfigProviders here into one.

Please also add some unit tests to verify that we can add additional aliases to the plugin managers while using the __construct. This would immediately uncover the problem I am talking about.

src/ConfigProvider.php

boesing · 2022-02-09T17:17:57Z

src/ConfigProvider.php

+    {
+        return [
+            // @codingStandardsIgnoreStart
+            'Zend\View\Helper\Asset'               => Helper\Asset::class,


I'd prefer to finally drop zend bridge functionality rather than adding these here one-by-one

This is solved in 3.0, but the changes here would need to be addressed in 3.0 also. If you think it's OK to kill off all the Zend stuff in a minor, then I'm happy to incorporate that into this patch

boesing · 2022-02-09T17:19:20Z

src/Helper/Navigation/ConfigProvider.php

+ * @psalm-import-type ViewHelperConfigurationType from \Laminas\View\ConfigProvider
+ * @psalm-suppress DeprecatedClass
+ */
+final class ConfigProvider


Components only have exactly one ConfigProvider.
If this component does also provide view-helpers, it has to be added to Laminas\View\ConfigProvider.

I'll just delete the second config provider - The navigation helpers subclass the main plugin manager and use specific config. There should really be a factory for this plugin manager that fetches the helper setup from 'config' which should not be listed under view_helpers but something specific to the nav plugin manager.

boesing · 2022-02-09T17:22:04Z

src/ConfigProvider.php

+     *
+     * @return array<string,string>|array<array-key, string>
+     */
+    public static function defaultViewHelperAliases(): array


Please mark this as @internal since there should be no reason to use this outside of this repository.
This also enables us to remove it in the next major.

Therefore, adding @deprecated would be helpful so we can find it when preparing the next major.

#110 (comment)

src/ConfigProvider.php

boesing · 2022-02-09T17:24:08Z

src/Helper/Navigation/PluginManager.php

@@ -81,6 +48,7 @@ public function __construct($configOrContainerInstance = null, array $v3config =
            $instance->setServiceLocator($this->creationContext);
        };

-        parent::__construct($configOrContainerInstance, $v3config);
+        $this->aliases   = ConfigProvider::defaultViewHelperAliases();


I haven't checked this due to laziness, but the aliases which were registered must not change in any way. Juggling between aliases/factories introduces BC breaks as we learned here:

laminas/laminas-mvc#99

If you can confirm that the factories and the aliases are 100% the same as in the previous version, we're fine here.

All of the factories and aliases were the same, but since adding extra tests, they've uncovered issues with the original config such as a factory for 'somenormalisedname' => InvokableFactory::class which of course blows up if you attempt to fetch by that name. I've amended these so that they alias the correct target - effectively a bug fix.

Considering they could never have worked in the first place, it might be a good idea to just remove them completely instead.

boesing · 2022-02-09T17:24:48Z

src/Helper/Service/IdentityFactory.php

        }

        return $container->has(AuthenticationServiceInterface::class)
            ? $container->get(AuthenticationServiceInterface::class)
-            : ($container->has(\Zend\Authentication\AuthenticationServiceInterface::class)
-                ? $container->get(\Zend\Authentication\AuthenticationServiceInterface::class)
+            : ($container->has('Zend\Authentication\AuthenticationServiceInterface')


I stay with it: we should just remove this bridge stuff in the next minor anyway

This is all gone in 3.0, so it makes sense to me to leave it in 2.x

src/HelperPluginManager.php

boesing · 2022-02-09T17:27:19Z

src/Helper/Navigation/PluginManager.php

@@ -81,6 +48,7 @@ public function __construct($configOrContainerInstance = null, array $v3config =
            $instance->setServiceLocator($this->creationContext);
        };

-        parent::__construct($configOrContainerInstance, $v3config);
+        $this->aliases   = ConfigProvider::defaultViewHelperAliases();


by adding this "after" the aliases, you might override additional aliases which were passed via config construct argument.

so you should either use array_replace_recursive($defaults, $configFromConstruct) or set the properties before actually calling __construct.

A unit test to verify that everything works as expected would be very appreciated.
especially for this kind of refactoring.

gsteel · 2022-02-09T21:44:44Z

Thanks for the review @boesing - I don't think it is possible to keep BC here because of inheritance. Both of the shipped plugin managers previously had default properties listing all the default configured aliases and factories - the existing behaviour internally is that the navigation plugin manager replaces these.

If anyone extends the default helper manager, by replacing the default values for those properties (which is a massive foot gun), this pull would blow away that information. Also, performing a merge with default config and the extended property values breaks existing behaviour.

Whilst this can mitigated internally (i.e. in navigation) by fetching the config and calling parent::__construct() at the right moment, I think it would be better to revert changes to the plugin managers themselves and break it all completely in 3. I think that a sealed, simplified plugin manager such as that described in #123 would solve all of this as ultimately, all the manager needs to do is make callables available to the templating layer in a configurable way.

I suggest that I put the default property values back to how they were, duplicate that information in the config provider and expose it via the component installer as described.

In 3, we can address the problems with the non-final plugin managers and remove all the junk.

What do you think?

- Introduces a descendant of the plugin manager to test that aliases and factories defined in the constructor are not overwritten - Adds a test to make sure that the plugin manager can retrieve all of the default configured helpers, skipping those that require additional dependencies Signed-off-by: George Steel <george@net-glue.co.uk>

Makes all config provider methods private except for `__invoke()` Reveals that existing v2 normalized factories do not work. Signed-off-by: George Steel <george@net-glue.co.uk>

…vice manager days Signed-off-by: George Steel <george@net-glue.co.uk>

Signed-off-by: George Steel <george@net-glue.co.uk>

gsteel · 2022-02-15T12:37:01Z

I'm closing this for now… Whilst I think this is a useful addition, It's got into a bit of a mess with noisy commits and there are a couple of things that I think should be fixed prior to the introduction of a config provider in the 2.x series:

Declare hard-dependencies related to service manager / psr-container etc
Fix or remove the legacy normalised aliases so they can actually produce a helper from the plugin manager

If anyone would prefer to see this go into 2.x, I'm ofc still happy to do the work, in which case feel free to re-open.

boesing · 2022-02-15T13:54:43Z

I totally understand the reason behind this change but as you said, the quirks, the BC breaks and all that stuff should be part of a major release to avoid unnecessary issues.

Happy to be of help somewhen in the next months or so. We can bounce some ideas and see how to properly implement something useful for 2.x

gsteel changed the base branch from 2.20.x to 2.19.x January 11, 2022 13:26

gsteel force-pushed the config-provider branch 2 times, most recently from 3425442 to d301c53 Compare January 11, 2022 13:47

Ocramius added the Enhancement label Jan 12, 2022

Ocramius added this to the 2.20.0 milestone Jan 12, 2022

Ocramius changed the base branch from 2.19.x to 2.20.x January 12, 2022 16:36

gsteel force-pushed the config-provider branch 2 times, most recently from 192b119 to 5c34b2c Compare January 12, 2022 18:25

This was referenced Jan 13, 2022

Add Composer Require Checker configuration #112

Closed

[RFC]: Ship factories, config providers and make use of plugin-installer #121

Open

gsteel force-pushed the config-provider branch from a57dd1e to d756c59 Compare February 1, 2022 14:00

gsteel added 6 commits February 9, 2022 15:19

Introduces Config Providers to improve types, ease integration and vi…

2bedffd

…sibility of default settings Signed-off-by: George Steel <george@net-glue.co.uk>

Updates baseline

8798736

Signed-off-by: George Steel <george@net-glue.co.uk>

Updates service manager to minimum version of 3.6 and move it to `dep…

438b6f9

…endencies` as the plugin managers cannot work without it. Signed-off-by: George Steel <george@net-glue.co.uk>

Adds both psr container and the legacy interop container to declared …

3f61968

…dependencies because they are both referenced directly in the code base Signed-off-by: George Steel <george@net-glue.co.uk>

Stringify Zend related class references so that composer-require-chec…

3beeef6

…ker doesn't complain about them Signed-off-by: George Steel <george@net-glue.co.uk>

chore: Update lock, post rebase on 2.20.x

a574c35

Signed-off-by: George Steel <george@net-glue.co.uk>

gsteel force-pushed the config-provider branch from d756c59 to a574c35 Compare February 9, 2022 15:23

Ocramius requested review from weierophinney and boesing February 9, 2022 15:38

Ocramius assigned weierophinney, boesing and gsteel Feb 9, 2022

boesing requested changes Feb 9, 2022

View reviewed changes

gsteel added 2 commits February 9, 2022 21:56

Revert changes to the helper plugin managers

b6efb34

Makes all config provider methods private except for `__invoke()` Reveals that existing v2 normalized factories do not work. Signed-off-by: George Steel <george@net-glue.co.uk>

gsteel added 5 commits February 9, 2022 22:37

Fixes the aliases for the legacy normalised service names from v2 ser…

7bbf015

…vice manager days Signed-off-by: George Steel <george@net-glue.co.uk>

Remove all legacy aliases from the proposed config provider

3828331

Signed-off-by: George Steel <george@net-glue.co.uk>

Fix introduced psalm errors

9462fc8

Signed-off-by: George Steel <george@net-glue.co.uk>

Removes unused use

1ae457a

Signed-off-by: George Steel <george@net-glue.co.uk>

Update composer with config provider and allow plugins for phpcs

39a41a0

Signed-off-by: George Steel <george@net-glue.co.uk>

gsteel closed this Feb 15, 2022

froschdesign removed this from the 2.20.0 milestone Feb 16, 2022

gsteel mentioned this pull request Apr 6, 2022

Introduces an Empty ConfigProvider and Module #163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces Config Providers #110

Introduces Config Providers #110

gsteel commented Jan 11, 2022

gsteel commented Feb 9, 2022

Ocramius commented Feb 9, 2022

gsteel commented Feb 9, 2022

boesing left a comment •

edited

Loading

boesing Feb 9, 2022

gsteel Feb 10, 2022

boesing Feb 9, 2022

gsteel Feb 10, 2022

boesing Feb 9, 2022

gsteel Feb 10, 2022

boesing Feb 9, 2022

gsteel Feb 10, 2022

boesing Feb 9, 2022

gsteel Feb 9, 2022

boesing Feb 9, 2022

gsteel commented Feb 9, 2022

gsteel commented Feb 15, 2022

boesing commented Feb 15, 2022

Introduces Config Providers #110

Introduces Config Providers #110

Conversation

gsteel commented Jan 11, 2022

Description

gsteel commented Feb 9, 2022

Ocramius commented Feb 9, 2022

gsteel commented Feb 9, 2022

boesing left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsteel commented Feb 9, 2022

gsteel commented Feb 15, 2022

boesing commented Feb 15, 2022

boesing left a comment •

edited

Loading