[ZF3] [RFC] Removal of get([A-Z][A-Za-z]+)Config methods from modules

[Ocramius]

This is an RFC for a simplification that would help a lot in building ZF applications considering non-cacheable configurations.

Problem

Some configurations rely on closures, service instances and generally non-serializable data that may break the application when config is merged and then cached

Current solution (ZF2)

In order to solve the problem, ZF2 introduced a series of interfaces that allow developers can implement in their Module classes to add configured services programmatically:

• Zend\ModuleManager\Feature\ControllerPluginProviderInterface
• Zend\ModuleManager\Feature\ControllerProviderInterface
• Zend\ModuleManager\Feature\FilterProviderInterface
• Zend\ModuleManager\Feature\FormElementProviderInterface
• Zend\ModuleManager\Feature\HydratorProviderInterface
• Zend\ModuleManager\Feature\InputFilterProviderInterface
• Zend\ModuleManager\Feature\RouteProviderInterface
• Zend\ModuleManager\Feature\SerializerProviderInterface
• Zend\ModuleManager\Feature\ServiceProviderInterface
• Zend\ModuleManager\Feature\ValidatorProviderInterface
• Zend\ModuleManager\Feature\ViewHelperProviderInterface

These configurations basically merge data to the already merged config AFTER it is cached. They all act in the same way, just with different naming and providing different config keys. That's a lot of duplicated code, triggered events and inconsistencies. Additionally, we never suggest usage of those methods if not strictly needed, which makes their existence very confusing for those who don't know that.

Suggestion (ZF3)

I hereby suggest removing (deprecating?) all the interfaces mentioned above, and instead introduce something like Zend\ModuleManager\Feature\UnCacheableConfigProviderInterface (suggestions on the name, please!):

namespace Example;

use Zend\ModuleManager\Feature\ConfigProviderInterface;
use Zend\ModuleManager\Feature\UnCacheableConfigProviderInterface;

class Module implements ConfigProviderInterface, UnCacheableConfigProviderInterface
{
    public function getConfig()
    {
        return [/* like before, nothing to see here! */];
    }

    public function getUnCacheableConfig()
    {
        return [
            // replaces `getViewHelperConfig()`
            'view_helpers' => [
                'services' => ['foo' => new SomeHelper()],
                'factories' => [
                    'bar' => function () {/* can't cache this! */},
                ],
            ],
        ];
    }
}

Bonus

With this approach, we can also get rid of Zend\ModuleManager\Feature\LocatorRegisteredInterface, which allows us to have modules like EdpMarkdown register the module as a service:

namespace Example;

use Zend\ModuleManager\Feature\UnCacheableConfigProviderInterface;

class Module implements UnCacheableConfigProviderInterface
{
    public function getUnCacheableConfig()
    {
        return [
            'service_manager' => [
                'services' => [__CLASS__ => $this],
            ],
        ];
    }
}

Benefits

• we get rid of a lot of triggered events during module loading
• we get rid of a lot of documentation related to those get([A-Z][A-Za-z]+)Config methods
• we get rid of Zend\ModuleManager\Feature\LocatorRegisteredInterface
• we get a minor performance improvement
• we get a clear understanding of what getUnCacheableConfig does. The name says it for itself
• we don't need to add more methods in case more plugin managers are added in future

ZF2 BC compatibility

In order to keep BC with ZF2, we can create a Zend\ModuleManager\Listener\Zf2CompatibilityListener, which emulates the same logic of all the current get([A-Z][A-Za-z]+)Config methods. It can be either on by default or opt-in, but that has to be decided

Drawbacks

None so far

TODO

• discuss naming of getUnCacheableConfig
• discuss naming of Zend\ModuleManager\Feature\UnCacheableConfigProviderInterface
• find possible drawbacks
• get positive or negative feedback (positive feedback gained)
• discuss Zend\ModuleManager\Listener\Zf2CompatibilityListener being turned on or off by default
• provide pull request

Conclusions

• get([A-Z][A-Za-z]+)Config will be removed from the module classes. getConfig will be used
• Using un-cacheable config will simply be penalized, avoid that. That's it, nothing more
• We will simply build a listener that verifies if config can be cached safely before attempting caching. That would allow us to throw meaningful exceptions
• We will provide a listener for ZF2.x compatibility, so that get([A-Z][A-Za-z]+)Config can still be used

That's it, only removal of of methods, and that's perfectly fine. I'll work on a PR as soon as possible.

[GeeH]

I'm all for this. 👍

[EvanDotPro]

Discussed this with @Ocramius in #zftalk and I am 👍. This would also simplify training (or new developer learning curve in general) around these concepts.

[kingcrunch]

VolatileConfig?

[macnibblet]

Zend\ModuleManager\Listener\Zf2CompatibilityListener should be disabled by default, we should not enable legacy behavior in new zf3 applications.

else 👍

[tfountain]

How about getRuntimeConfig() instead of getUnCacheableConfig()?

[akrabat]

getUnCacheableConfig will get very big if it combines the config in all the currently separate getXxxConfig() methods. Is the idea that we discourage creating closures and make everyone write separate factory classes for every class that needs injection?

[Thinkscape]

Sounds nice.

@akrabat I doubt you'll ever see that resolved. From my personal experience, a single feature I wrote yesterday required 15 factories depending on each other. It's just much quicker to write closures than to write separate files with factory classes. There's also the benefit of navigation (I can easily see all my 3-5 line factories at a glance, as opposed to opening each separate file). (of course a proper DIC and build would solve it all, but I digress).

The only drawback, (per @akrabat ) I see, is moving again in the direction of arrays-of-arrays-of-arrays which emulate duck-typing, instead of feature-based loading, interfaces and OOP structures.

For example, explaining "in order to add view helper, define getViewHelperConfig() method" you'd be saying "shove it into that big getSomethingConfig() method, but watch out, don't confuse it with getConfig() method". Whatcha say professor @EvanDotPro ? :-)

I'd vote for either getDynamicConfig() or getRuntimeConfig().

[EvanDotPro]

getUnCacheableConfig will get very big if it combines the config in all the currently separate getXxxConfig() methods.

For the sake of sanity, I would probably suggest something like...

public function getUncachableConfig()
{
    return array(
        'service_manager' => $this->getServiceConfig(),
        'view_helpers' => $this->getViewHelperConfig(),
        // etc
    );
}

Which could also produce a backwards-compatible module.

Is the idea that we discourage creating closures and make everyone write separate factory classes for every class that needs injection?

In my eyes, this is not the purpose. I see nothing wrong with closures and direct services being registered directly in the module class, especially in very, very simple cases like EdpMarkdown. Personally, I think doing it this way simply makes it easier to understand how and why it works the way it does. Right now, you see a lot of people defining things like invokables, etc in getXyzConfig(), which is a waste. Additionally teaching a new developer how the different methods merge into the different config keys after caching is a hassle, whereas this strategy makes it very clear what's going on. Just my $0.02.

[EvanDotPro]

@Thinkscape yes, I agree about it being easier to explain. As I mentioned to @Ocramius on IRC:

02:46 < EvanDotPro > ocramius: and it'd be easier to explain than "getServiceConfig() is merged under service_manager... getViewHelperCOnfig() is merged under view_helpers" etc

[akrabat]

@Thinkscape Yes - I'm not a fan of creating a factory classes for a 3 line method either, so do the same as you and am happy with that. I like @EvanDotPro's idea of getUnCacheableConfig() being used to marshall the other methods.

[EvanDotPro]

I haven't formulated a final opinion yet, but I'm currently leaning towards something like getUnCacheableConfig() as opposed to getDynamicConfig() or getRuntimeConfig().

While not as "fancy" sounding as the alternatives, getUnCacheableConfig() quite clearly states the significance of the method, whereas it has to be implied with the others.

[akrabat]

It should be getUncacheableConfig() rather than getUnCacheableConfig().

I don't especially like getUncacheableConfig(), but I agree with @EvanDotPro that it's very clear. However, if we're going that way, then getConfig() should probably become getCacheableConfig()…

[Ocramius]

I don't think that the size of getUnCacheableConfig() is a problem. I've always dealt with it even for getConfig() by splitting all in different files, such as:

public function getConfig() // works also for getUnCacheableConfig
{
    return [
        'service_manager' => require __DIR__ . '/../../config/service_manager.config.php',
        'view_gelper' => require __DIR__ . '/../../config/view_helpers.config.php',
        // ...
    ];
}

Just a note on the method naming: please suggest something that is easy to understand.

getUnCacheableConfig() or getLateConfig() or something like that may work.

getRuntimeConfig() suggests a mutable config, which I personally don't think is a good idea. Same for getDynamicConfig(). While making config mutable depending on I.E. the request object is possible, I see it as a possible source of bugs.

[Thinkscape]

getUncacheableConfig() ... it's a mouthful, there's a spelling requirement, this will cause a lot of trouble and wtf moments. Better use something easier to spell.

getUncacheable getStorable etc. - also seems like trying to explain implementation details - who cares if it's going to be cached or not.

Possible questions:

Q: is getUncacheableConfg() method being invoked in development, when caching is disabled ?
Q: does caching change the order of merging of getCached/getUncacheable config ?

I'd hope to have a name that is more about the semantics and architecture than what the MM is going to do with it later (caching, serialization etc. should not be part of module definition).

[Ocramius]

Q: is getUncacheableConfg() method being invoked in development, when caching is disabled ?

@Thinkscape yes, getUncacheableConfig() is always called

Q: does caching change the order of merging of getCached/getUncacheable config ?

No. getUncacheableConfig() is always called AFTER merged config was cached or loaded from cache

[Thinkscape]

Marco, I know answers to those questions, I'm pointing out the ambiguity of the name and possible questions that such method name could raise :-)

[Ocramius]

@Thinkscape got it. Yes, I don't have better names so far.

[akrabat]

Calling it the uncacheable config isn't unreasonable as the only reason that it exists is that getConfig() cannot have closures in it's returned array.

[Ocramius]

@akrabat yes, that's the main reason for it to exist (and it's the same reason for the get([A-Z][A-Za-z]+)Config methods to exist)

[bakura10]

I'm for removing completely those in ZF3, for the only reason that it allows us to remove this crap.

By the way, still no zf3 branch/repo ? :(

[samsonasik]

@bakura10 this empty repo ? https://github.com/zendframework/zf3

[Thinkscape]

We could always reverse it, so getConfig() and getCacheableConfig()... still a mouthful though. @akrabat you don't get a say because you're British and you love long and hard to pronounce or spell words :-)))

[bakura10]

@samsonasik , this repo has been here for months. But still empty for months too :).

[Ocramius]

Changing meaning of an existing method would be a no-go. That's much more of a BC in my opinion. Think carefully and look for a better name first.

@bakura10 I'm still against a separate repo for zf3, but this is not the thread for it. Focus on the issue, please

[macnibblet]

I think everyone is against a seperate repo for zf3

[Thinkscape]

getRuntimeConfig() suggests a mutable config, which I personally don't think is a good idea. Same for getDynamicConfig(). While making config mutable depending on I.E. the request object is possible, I see it as a possible source of bugs.

You're right.. however it doesn't give me much more hint than existing getConfig() which could also be used in that fashion. getRuntimeConfig() looks nice when you put it besides getCachedConfig() (because word "cached" seems opposite to runtime). Or getCallableConfig() ?

Current contenders

public function getConfig();
public function getRuntimeConfig();
public function getDynamicConfig();
public function getCachedConfig();
public function getUncachedConfig();
public function getCacheableConfig();
public function getUncacheableConfig();
public function getUnCacheableConfig();

[Thinkscape]

@Ocramius If you consider we won't be using serialized closures, getDynamicConfig() makes sense to me (it's dynamic in sense of using scoped, referenced vars and instances with closures, which all change their addresses with each script invocation)

[Ocramius]

@Thinkscape super_closure is an experimental project. I wouldn't use it anyway ;)

Dynamic can do for me as well. I just hope people don't abuse it =D

[EvanDotPro]

@bakura10

I'm for removing completely those in ZF3, for the only reason that it allows us to remove this crap.

You mean removing it completely without a getUncachableConfig() alternative? That would mean I cannot create clever, single-file modules like EdpMarkdown that fit on a slide for a talk, so absolutely not. ;)

By the way, still no zf3 branch/repo ? :(

I was put in charge of starting this, but it created a large debate on how it should be handled (branch, vs separate repo, etc), and there was no consensus reached, so I postponed the new repo for now. We'll kick it off soon, but in the meantime, the WIP/RFC issues and PR's here in the ZF2 repo are working perfectly fine.

We could always reverse it, so getConfig() and getCacheableConfig()

Definitely 👎 from me on this, sorry.

I'm also still 👎 for getDynamicConfig() and getRuntimeConfig()`. I can reject names all day, just don't ask me to suggest any. ;)

[bakura10]

No Evan, I want the getUncacheable thing. I ust want to remove all the various getXXXConfig and just keep two ones.

[texdc]

@Ocramius Wouldn't that config be written without the DateTime instances?

['report_timespan' => ['start' => '-1 month', 'end' => '+1 month']]

[Ocramius]

@texdc good point - I will need to think about that!

[Thinkscape]

This is basically a concept of a piece of config that is mutable. I know I stated before that config should not be mutable, but this unlocks quite some possible development directions.

Like with the datetime example, keep in mind that many of those cases should be handled by callables (i.e. instances created on-demand of validators, services etc.). Similar to the diff between factories and services under SM config - factories and closures are cheap at definition time, expensive when calling, and you shouldn't instantiate anything unless you really need it. The more non-app modules keep doing that, the heavier the app will become and we loose the whole modularity advantage of zf :-|

[Thinkscape]

Not entirely true. In some cases, it may be used for the services key as well, for actual instances:

@EvanDotPro maybe this should be split out then ? There's a difference between uncacheable config, cacheable config and not-a-config :-) This would give us 3 concerns instead of 2.

We could also think about calling it getServices(), which logically groups the kind of things that we have trouble with. Everything else is inherently cacheable and with little probability of containing closures.

[Ocramius]

It's not just about services. Anything not cacheable would fall in here.

[EvanDotPro]

getNotCacheableConfig()? Just thought I'd throw another one into the pool. I'm not really sold on it, just came to mind.

[stefanotorresi]

I raise on getBareConfig() :p

[Thinkscape]

getBearConfig()​

[GeeH]

getConfigThatCantBeCachedBecauseItsProbablyAClosure()

On Fri, Oct 25, 2013 at 9:21 AM, Artur Bodera notifications@github.comwrote:

getBearConfig()

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/5288#issuecomment-27073078
.

[glen-84]

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

What if you avoided this completely, by using an event listener, as we do for bootstrapping (onBootstrap).

If you want to use closures (or anything else that cannot be cached), you would implement the ConfigListenerInterface (onConfigLoaded). This would be triggered by the module manager and passed the merged (and usually cached) configuration.

From here you could make additional contributions to the configuration object as necessary.

[mschindler83]

What about getPostCacheConfig() ?

[Ocramius]

@mschindler83 very confusing for web developers...

[EvanDotPro]

@glen-84

What if you avoided this completely, by using an event listener, as we do for bootstrapping (onBootstrap).

If you want to use closures (or anything else that cannot be cached), you would implement the ConfigListenerInterface (onConfigLoaded). This would be triggered by the module manager and passed the merged (and usually cached) configuration.

From here you could make additional contributions to the configuration object as necessary.

We already have an event for that, actually. However, it should not be used for this purpose, as any configuration provided via such a listener will not be overridden by global/local autoload configuration. IMO the global/local config files need to always the be "winners" of the entire merging process.

The primary use-case for the existing event is unsetting config keys that are defaulted by vendor modules (removing ZfcUser's routes for example), though I've seen a few other uses such as token processing, etc.

[Maks3w]

We had a discussion at ZCEU about this:

I've explained my proposal of split cacheable and not cacheable config values iterating over the merged config and then store each one in the best storage possible (cacheable things in opcode caches and not cacheable things in a PHP file written in disk)
In successive runs of the app the config is retrieved from both sources (cacheable storage and not cacheable storage) and merged.

@Ocramius Notice the problem with the closures context (use keyword) may difficult to preserve in the non cacheable storage.

So we think that in source of the question config should be static values and there are no options for non cacheable things beeing the correct way for that use cases write a service with the aim of generate a dynamic config from static parameters (ServiceA inject ServiceAConfig)

We could add a trigger_error warning for non cacheable values so the user could be noticed when are not making a config in the correct way.

If I forgive something of our discussion please comment. /cc @Ocramius @EvanDotPro @akrabat

[Ocramius]

No, that pretty much sums it up.

Current solutions:

• Have a separate method for un-cacheable config
• Don't support non-cacheable config (at all) - throw a meaningful exception while trying to cache config instead

The example with a special config key doesn't work at all anyway.

Right now I'd just suggest dropping all those get[A-Z][A-Za-z]+Config methods and that's it. No kind of support for non-cacheable config.

[bakura10]

👍

[Ocramius]

I wrote down the conclusions of the brainstorming I had with @Maks3w as well as what has been discussed here. That's what I'm gonna implement, so please read now or forget about it later on :-)

[Maks3w]

👍

[GeeH]

This issue has been closed as part of the bug migration program as outlined here - http://framework.zend.com/blog/2016-04-11-issue-closures.html