-
Notifications
You must be signed in to change notification settings - Fork 3
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
Support "deferred" modules #12
Comments
Why not adding a Proxy to handle that? With a Proxy any service will not be instantiated untill some method will be executed so they are deferred by default, good for WordPress Events. I implemented such system here https://github.com/ItalyStrap/empress/blob/master/src/ProxyFactory.php and I pass this callable to a modified version of Now to declare a service as a proxy I only need to add the For doing so I use a library made by Ocramius https://github.com/Ocramius/ProxyManager this library is very strict about PHP version but there is also a fork with LTS support https://github.com/FriendsOfPHP/proxy-manager-lts And if you are concern about performance you can tuning it as well https://github.com/Ocramius/ProxyManager/blob/2.15.x/docs/tuning-for-production.md |
@overclokk A proxy can not fully help here. The problem is that the services right now must be added to the ContainerConfigurator before "init". After that hook is called, the container configurator creates the container which is read-only. At that point no services can be added anymore. When I say "the services must be added" I'm not referring to service instances, but to services factory callbacks. That is, callbacks that when executed will instantiate the service. To reinforce: all services are already "lazy". When you register a service, it is not instantiated at all. It is only instantiated when/if another service requires it. It is already "kind-of proxied". This issue aims at reducing the memory footprint because even if lazy, the factory callback must be stored in memory, just like the proxy would need to be created and stored in memory, so there's no way we can reduce memory footprint via proxy. The key point here is that the read-only container we use is very strict, and it forces us to register everything upfront. Registering a factory callback or registering a proxy is not much different conceptually. If the read-only container would allow us to register factory callbacks after "init", yes, we could achieve this via a proxy. That would have the benefits that the proxied service could be passed to other services before it is ready. But it would hve the disadvantage that it would be more memory-heavy, besides adding a 3rd party dependency to a package that is used pretty much in every other package in the company, and that does not sound like a great idea to me. So IMO the thing to solve here is make the read-only container nor read-only anymore, but maybe append-only, and possibly locked from the external. That is a pre-requisite to make any use of a proxied service. But, to be honest, if I realize I need the service to be injected to other service before a certain hook, I can just register the service upfront as we do now. The only case in which a Proxy would at that point be useful, is that a service instantiation is heavy, but that is something we should solve by having constructors that are mostly noop, not encoraging proxies, especially when that means to introduce dependency to very opinionated 3rd-party dependencies. |
I get the spirit of this idea and it's actually something I've been discussing a few times in the recent past. My TLDR is: I would like to collect and see data that supports the notion that there is a problem with "over-registering" services. Long answer: I actually see the "rigid monolith" as an aid in the sometimes confusing multi-context environment that is WordPress. While you could technically already have some dynamic components by conditionally adding/withholding modules and connecting Packages, one core aspect remains once Everything that is declared is also available. This makes it very straightforward to conceptualize your "large library of services". If you get a I realize I'm arguing very subjectely and gut-feely here. This is because I quite fully agree with the reasoning behind the feature proposition on a technical level, but I have concerns that it is a "solution looking for a problem". So do we have data that suggests we need to invest in this? And are the potential benefits substantial enough to introduce such a paradigm shift in the way we look at the list of declared services? If we have that data: Maybe there is a way to do this transparently? Imagine we'd have a build/caching step for production environments. It could essentially When a service is requested, we would check this map and internally load additional declarations from modules if they're not present already. This would
This would sufficiently defer the loading of excessively large modules into memory without any new user-land code and it might even cover scenarios that would otherwise require an exceptionally steady hand in assembling/configuring modules & declarations. ¹ it's an array because a service can be overridden. Maybe we could simply use the last Module as a single string though. It's just a pencil sketch |
Yes, I know this, I had the time to study the code for one of ours project.
Ok, if you talk about only the registration part ok, I'm with you (I hate all that boilerplate stuff), if you do not have some autowiring mechanism you need to register all services before they are called.
Conceptually does not but tecnically yes, they are different beast. $service = $c->get('SomeService'); // This give you the real service
$service instanceof 'SomeService';
...
$c->proxy('SomeService', fn() => do stuff);
$service = $c->get('SomeService'); // This give you the "fake" service
$service instanceof 'SomeService'; // Fafe service with correct Interface|whatever
// do some stuff in you application
$service->callRealMethod(); // At this point the real object is instantiated with all dependency trees.
Maybe this could also be solved with reflection?
Not only for that, becuse the nature of proxies, if you register one Proxy for
You don't need to add another dependency, you only need to add a "suggest" key in At this point I propose also the Autowiring concept, with this you can save memory because you do not need to register every service in the application but only a few (for example services that need to be shared so only one instance is created.) And you will say, "But that use the Reflection..." and I answer,yes, but the reflected obect could|should be cached in some way. I use Time ago I worked on a PR for implementing the Proxy pattern in Auryn\Injector but was not merged because there was no interest in that, it was merged in another forked package and also I kept the code and I use my own version for that reason. |
For today ID I made two experiment, one with a simple reflection for the I made Reflection only for fun, but if you have to add this just make the The other experiment was with an Autowiring container and the ProxyManager, for autowiring I use Auryn\Injector (my forked version). Injector needs to be "configured" only for a few things, aliasing Interface to Concrete, Sharing if you need to share something and some less used things, anyway most of the configuration happens with string and not with factories at least if you need those in some scenario. The simple implementation here explain what I did: function injectorPreparation(): \ItalyStrap\Empress\Injector
{
$injector = new \ItalyStrap\Empress\Injector();
$injector->share($injector);
$injector->alias(\ItalyStrap\Config\ConfigInterface::class, Config::class);
$injector->share(Config::class);
$injector->proxy(ExampleWithDependency::class, new ProxyFactory());
return $injector;
}
/**
* Provide the plugin instance.
*
* @link https://github.com/inpsyde/modularity#access-from-external
*/
function plugin(): Package
{
static $package;
if (!$package) {
$injector = injectorPreparation();
$properties = PluginProperties::new(__FILE__);
$package = Package::new($properties, new Container(
$injector,
$injector->make(\ItalyStrap\Config\ConfigInterface::class)
));
}
return $package;
}
After that I created a module and inside the $example = $container->get(Concrete\ExampleWithDependency::class);
\var_dump($example); The dump value is not the real object but the proxy object and the dependency is never instantiated, only if you call BTW: The `services()' was empty, no need to configure factories here. Now if you need more in depth examples or performance test let me know and I'll try to add those. |
I still have not understood how proxies and autowiring are relevant to the issue presented in the OP. Let's say your project contains a Whether or not a module's services are autowired and/or return proxies is not relevant. Am I missing something? Don't get me wrong, I have a fair bit of experience writing and consuming autowired containers and I've written a proxy library myself in order to transparently break circular dependency chains during autowiring. It's awesome! But it's not the point here. |
At this point I think you have another kind of problem.
I don't get why is not relevant. Give me some real code where I can do benchmark, just for fun. |
This issue describes an additional funcitonality that I think would be nice to have, based on the experience of using WP App Container for plugins and from discussion with colleagues trying to implement modularity in their plugins.
The scenario
Let's imagine we have a simple plugin that needs to perform a couple of operations.
The first operation must be done pretty early, let's say on
"init"
.The second operation can't happen that early, because it depends on a condition that is only possible to check at a later hook.
The plugin could use two modules, one per operation.
Let's ignore for now the first module, and let's imagine how the second module could look like.
This a quite common situation. We only need to do "things" on specific type of frontoffice request, but the first hook to safely check for conditional tags is
template_redirect
.The issue
Now, if in our website we have a request that satisfies our condition once every 1000 requests, it means that 99.9% of the times the module registers services in the container for no reason.
When the number of services registered is not trivial, register services might be resource consuming, because pollutes memory and trigger autoload (which might be slow file-based operation).
When
template_redirect
runs, it is too late to add things in the container, because the pluginboot
is executed earlier (because there's a plugin functionality that needs to run earlier).That's why we check the condition inside
run()
, but the container instance passed there is read-only, so we need to add services earlier.In short, the problem is that when we have a module that should register things conditionally, and the condition is not available very soon (quite common in WordPress), we are forced to register things unconditionally and then check the condition later.
The possible solution
I think that a possible solution could be to introduce a
DeferredModule
interface, similar to this:and a
Package::addDeferredModule()
method, with a signature like this:Internally, the method would add an action to the hook returned by
DeferredModule::deferredTo()
and inside that hook, it would callDeferredModule::factoryModule()
and the returned module, if any, would be added in the same way it is hadded when callingPackage::addModule()
.The code could look like this (it's an idea, completely untested):
With the above code in place, in the scenario I described a the beginning, the
MyTemplateRedirectModule
could be rewritten like this:In this case the deferred module return itself from
factoryModule()
, but it could return another module.Anyway, having the proposed code in place, by calling the following line the issue is solved in a pretty simple way.
For requests that are not archive of a specific post type, not only the module does not execute anything, it also does not register anything.
Additional notes
Considerations on
addDeferredModule()
The code proposed above for
addDeferredModule()
is simplified. There are other things to consider, e.g.:$priority
parameter inaddDeferredModule
, or maybe have anotherPrioritizedDeferredModule
interface which would extendDeferredModule
with an additionalpriority()
method.add_filter
instead ofadd_action.
We could accept an additional$isFilter
parameter inaddDeferredModule
, or maybe have anotherFilterHookDeferredModule
interface which would extendDeferredModule
to distinguish actions from filters.addDeferredModule()
, but use additioanl interfaces for the two reasosn exposed in the previous two points, theaddDeferredModule()
would have the same signature ofaddModule()
so they can be merged in a single method, and based on the interface apply the deferred workflow.Deferred modules for inter-module dependency
Deferred modules could be used for another use case, thanks to the fact that the
DeferredModule::factoryModule()
receives an instance of the package.Imagine a module that should be added only if another module is present.
Package
class has themoduleIs()
method, but that can't be used onPackage::ACTION_INIT
to determine if a given package was added or not, because maybe the target package will be added later. And in any case, can't be used to check if a module was executed or not, because the execution happens later.With the addition of the proposed functionalities, it would be possible to do things like:
In the snippet above, I added a deferred module that when the package is ready (
ACTION_READY
action is fired) checks if theSomeModule
module was executed successfully, and only if so will add itself as an additional module.A similar workflow can be currently implemented. In fact, currently it is possible to do:
So we can currently execute
SomeModuleDependantModule
only ifSomeModule
was executed successfully, but we can't currently add services at that point, because whenACTION_READY
hook is fired, the container is read-only, so any service required bySomeModuleDependantModule
would need to be registered unconditionally.The text was updated successfully, but these errors were encountered: