-
Notifications
You must be signed in to change notification settings - Fork 1
Reconsider structural requirements #21
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
Comments
This, to me, seems to summarize the entire issue:
Firstly, the "Provider" naming scheme is something I'm tending more towards recently, so I'm all for it. It's more meaningful and the same terminology is also used in other languages. Secondly, I don't believe there is any actual need for Module aggregation is already a very app-specific thing just by its very nature: the app installs its preferred module aggregator (such as a custom composer installer) and then uses it to obtain a list of modules. The Though, I would go a step further and say that a module aggregator isn't even necessary at all. As you put it, the beauty of the standard is that it's just an interface, and implementers are just a class. Meaning that this alternative to module aggregation and bootstrapping works just as well: <?php // modules.php
return [
'core' => new CoreModule(),
'ui' => new UiModule(),
'db' => new DbModule(),
'migration' => new MigrationModule(),
'logging' => new LoggingModule(),
'wp_cron' => new WpCronModule(),
'rest_api' => new RestApiModule(),
]; Just as how And that's sort of the entire point. The above approach would be considered non-standard by the spec, but I believe the spec shouldn't have a say in this. So I agree; the only truly important part of this package is |
I agree that About I would consider adding support for discovery via Composer to something like @Biont, do you require the Composer config in order to be able to add e.g. IDE support? So, I would say for now: remove Thoughts? |
No, I simply iterate over any found implementations of And as I said, the composer config might as well be...
This is really something optional. But if it was suggested (not required) by the standard, it would serve as a great base for those sorts of applications that opt to use composer plugins for discovery/IDE-integration
Well this is not really true and it's the entire reason I've opened this issue:
(emphasis mine) And this is the very first actual sentence of the usage documentation. The use of an uppercase "MUST" (like most standard docs I'm aware of) strongly suggests that "If you don't do that, you breaking the spec". At the end of the day, all I really want is a clear separation between standard definition and suggested usage example. And I want to mention that while I'm sitting here complaining, I really appreciate the documentation/examples |
Thanks, @Biont! I apprecate your contribution and involvement! Yes, I'm all for that separation. To be precise, the "MUST" refers to the I think that the best way to go about such separation is to create an actual modular framework that allows quick application building using the modular system, and other Dhii and non-Dhii standards and implementations. Maybe, something like How does that sound? |
In fact, I had already created a repo where I try to retrospectively remove the WP aspects of |
Uh oh!
There was an error while loading. Please reload this page.
Concerns
I would really like you to reconsider the amount of structural conventions surrounding the use of
ModuleInterface
. As you know, I have spent some time toying with adding IDE support to modular projects via a composer plugin and one of the things I couldn't quite solve is dealing with constructor arguments. (->The plugin currently only inspects the bare implementations ofModuleInterface
without respecting the loading pattern viamodule.php
.So the natural thing would be to extend support to the broader spec laid out in the README, but I cannot stop complaining. Hence, this issue.
It feels to me as if this whole
module.php
pattern was mostly born out of the inability to safely bootstrap a collection of modules based on the composer package type alone¹ I think the beauty of the interface itself is that it's just 2 methods + the dependency on service-provider. But here you are blowing it up with a large amount of outside requirements:module.php
fileTechnically, only the
module.php
file is spelled out as a hard requirement that a module MUST fulfill, while the rest might serve as an "example implementation" without being declared as such.But
module.php
is such a weird (and weirdly specific) requirement to make, and it got criticised before: #16 (review)It's also somewhat contradicted by the existence of
ModuleAwareInterface
. Why add an interface that would allow modules to be retrieved from a class instance if you set in stone that it must be provided bymodule.php
. I don't think it makes sense to define two opposing approaches to retrieve a module instance here.Another thing that bothers me is that - while it would absolutely be possible to support
module.php
in the IDE helper- you still cannot be sure you can instantiate a module without having to satisfy dependencies that a composer plugin cannot provide.In other words: I think the goal of requiring a
module.php
file was to enforce a standard way of supplying module factories in order to steer around the requirements of any individual module's constructor arguments. But it does not and cannot enforce the factory function itself to be devoid of function parameters so this point falls flat a bit. I think requiring a factory at all is a pretty hefty part of the spec when this should ideally be each individual application's concern. And if you're gonna go there, I would really like to see a more flexible approach - ideally one that allows usage of the standard without a factory as wellSuggestions
I've been thinking of two things:
ModuleAwareInterface
so thatgetModule
is static²:module.php
as part of the standard, suggest that implementing packages SHOULD contain an entry in thecomposer.json
like this:"module-factory"
could be one of the three:ModuleAwareInterface
which can now be called statically (erasing the dependency problem in the safest manner)ModuleInterface
ModuleInterface
ModuleInterface
themselves to see if they can be instantiated without constructor arguments. That, or search the package for anyModuleAwareInterface
² and use that. In these scenarios, specifying this composer config would practically serve just as an override if the default behaviour is undesirable. Another reason why it would make sense to make this an optional requirement to begin withDoing this would allow trimming down the spec to its bare minimum again while still fully satisfying any existing workflows. AND you'd introduce a standardized way for composer plugins to integrate with the module packages. And this doesn't even require them to be of a specific package type any longer.
¹This is something the composer plugin could really help out with by simply dumping all modules found. Of course, this gets more complex as soon as you need to customize the load order, but maybe a solution can be found for that as well
² Maybe this should be renamed to
ModuleProvider
or put into a new class after all. I have not thought too much about the anticipated use-cases ofModuleAwareInterface
and just more or less assumed it's currently just sitting there unused by anyoneThe text was updated successfully, but these errors were encountered: