-
Notifications
You must be signed in to change notification settings - Fork 5
Initial #1
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
Initial #1
Conversation
It was never supposed to be committed anyhow
Deps updated
Not sure if this will work, because I don't know how Actions cache works yet.
Test included.
This includes a modules list file, an application bootstrap file, and a basic test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments that I wanted to leave are mostly cross-cutting - I couldn't decide what file or line I should leave my comments on. So I'll stick with my usual format of a single message with comments first and suggestions later.
Comments
I'm personally not a fan of bootstrappers, but I understand the appeal of quickly spinning up a new WordPress plugin project, especially for project-level things like Docker, PHPUnit, PHPStorm, etc.
That said, I have to ask the question: what problem does this package actually solve? To me it seems like there are multiple answers to that, and ideally there's only one per package. Which leads me to my next point:
It's not fair to call this package a "bootstrapper", since it not only provides project-level stuff but also provides runtime functionality such as the main plugin file, module loading, container construction, etc. If, hypothetically, I just want to use the ready-made plugin functionality and module runtime logic, I have to contend with the rest of the stuff that is bundled with it like Docker, PHPUnit, Psalm, etc. Not to mention that some of the code here isn't even WordPress-related and can genuinely be useful for other projects.
Finally, the module loading. I don't even know where to begin honestly; it seems so unnecessarily complicated. I'm also surprised to see that you went with a composite container approach, rather than a composite service provider. There are many clear benefits to the latter, most notably performance, easier compiling, works with any SP-compatible container implementation out of the box, and fits in well with the idea of having a root plugin module that "parents" the other modules. I'm even more surprised to see that you are still sticking with separating factories and extensions in their own .php files.
Suggestions
I would split this package up into multiple ones, be them WordPress-related or otherwise.
First off, the modules. This is probably going to be controversial due to how differently you and I see modules. Personally, I don't like to attach "extra" code to modules. What I mean is that a module is just a single class, not a package that bundles sources that specific to it. It's just a class that provides a preset for services, integrations with other modules and execution logic; a thin object that utilizes functionality that is already available on its own without necessitating the module.
For this reason, I don't see the need for the modules.local directory and the wikimedia/composer-merge-plugin dependency. For the most part, plugin modules will most likely exist within the same package as the plugin (speaking from experience). And when not, which is fine and I can see many use cases for, the module is just a class that resides in vendor that the plugin instantiates and uses.
At RebelCode, we keep a modules.php file in the root directory that looks something like this:
return [
'my_cpt' => new MyCptModule(),
'my_tax' => new MyTaxonomyModule(),
'logger' => new LoggerModule(),
];As you can see, it makes no difference where the module classes come from - they are all autoloaded equally. We also make an effort to not depend on the plugin's location on the file system when creating the modules - if modules need path info, a CoreModule will provide services for those.
Now onto module loading. I understand that this is not a simple one-liner; containers need setting up, the module list needs to be fetched, then providers need getting, and so on.
But solving that problem should not be the concern of this package. In a perfect world, there should be a one-line solution such that f(m) -> c where m is a list of modules and c is your container, or application (honestly, I'm of the opinion that this should be part of the module standard. But I know you disagree with that).
With such a package, all you'd need to do is composer install it, and use its provided logic in your plugin's main file.
// plugin.php
// The CoreModule I mentioned.
// Has aware of the plugin's location, and parents the other modules
// by acting as a composite module
$coreModule = new CoreModule(__FILE__, require 'modules.php');
// Any SP-compatible container will do
$c = new Container($coreModule);
// Run Forrest run!
$coreModule->run($c);Speaking of which, the plugin stuff. When it comes to doing things modularly, the plugin main file takes a backseat in that it does nothing except load stuff. But there are other useful features that modular plugins can leverage, such as accessing parts of WordPress using services, or having the plugin's services be filterable using WordPress hooks.
I would create a library for this stuff, that depends on the previously mentioned proposed package, that acts as a sort of "unofficial SDK" for WordPress plugins (something I'm actually working on myself). This package can provide simple one-liners that, when put in the plugin.php file, give you a working plugin.
Finally, you can use those in this package which on its own does not offer any runtime stuff, but merely gives you a good starting point with some bundled tools to boot. This way, you have a clean bootstrapper that you can use to spin up a ready-to-go development environment.
|
Most probably, your comment is a result of me not communicating the goals of this project clearly. I'll explain some of the decisions here, and hope it will help understand what I'm trying to achieve.
I think that the choices you are concerned with are quite justified. If a new plugin author doesn't want the large feature set provided by this package, they are free to start a plugin from scratch, and use any module loading method they want (even a 20-line one). This package tackles a very large amount of problems that regularly present when I create new enterprize-grade modular plugins. That said, if you have other problems associated with creating new modular plugins, or you believe that there's a better way of doing something here while maintaining the goal of the package, or you find a feature that really is extra, I am very open to ideas/suggestions. |
|
You are right. And I will explain at the end why, but first I must fulfill my duty as reviewer to leave comments
I can extend that logic quite easily into the following statement: All PHP Composer packages can be combined into just one package. Just use the classes you want and don't use the classes you don't want. Hopefully that demonstrates what I'm trying to say.
That's exactly what I mean. It has nothing to do with WordPress and is, quoting myself: "genuinely useful for other projects".
I hold a different, but not opposite, opinion. It's simply: who cares where they come from? They're just classes. For that reason, I don't see how the premise of module size/complexity leads to a dedicated package becoming desirable. So naturally, I can't see why a boilerplate package needs to be the package that solves that problem.
Again, I see what you're saying, but I don't see how the premise implies the conclusion you arrive to, that a
I have one. Will trade for nudes. Re. composite container, you brought up these points:
Well that sounds like application-specific logic. And in a modular application, all such logic is a module. So, make a module accept
Plenty of ways to do that, but mock objects would probably be best. But if you insist on composite containers, you can just create a composite container inside the test and give it your application container. Not that I'm endorsing that in any way. Use mocks.
Much like the first point, sounds like the job for a module. Maybe even a "core" module.
I agree with the premise, but I don't know what exactly is being "achieved" by the composite container. There's no scenario where adding a new container into the system is better than adding a new module. And if modules is all you have, you don't need the container to be composite. Just my 2 cents.
I strongly disagree. Every application will follow the same module loading procedure:
The only things that change are the steps before 1 and after 3. Yes it's small, but yet again I don't see how that leads you to the conclusion that it's not reasonable to create a reusable module system that provides a loading mechanism that achieves the above.
Well, this is not the right attitude IMO. While the statement is true and I do agree with it, I don't feel like it has a place in this discussion, unless it's being used defensively to justify being opinionated. Which leads me nicely into the conclusion: In the beginning I said that you're right and that I'd explain why. You said it in plain words: this is an opinionated package. And I have to respect that. There's no objective right or wrong - only a package that fulfills your needs as best as you can design it, with the hope that it attracts others with similar needs. So I can't argue much there. I'd take back everything I said in this comment, but I know you wouldn't appreciate that. At the same time ... I don't think my reviews are going to be of much help to you here. Our differing views on modules and package organization is going to conflict way too much, especially given the nature of the project. It's quite a match made in hell if you think about it 🤣 I at least hope that this comment is deserving of my role as reviewer, and that you got something out of it; even if it's just something that makes you pause and think, 6 months from now. |
No, that statement would not be a correct extension, because all PHP Composer packages are not something that is frequently used together.
Didn't understand how your reply agrees with or contradicts my point.
For the consumers it doesn't matter where modules come from. But for the author who will need to extract modules into separate packages - it does. You really haven't ever have to extract built-in code into a different package..?
There can be no such unit because, like I said before, all this stuff is quite application-level. There's nothing about the bootstrap process that can be re-usable, with the exception of perhaps
Deal! 🎉
It is application-specific logic. Which is why it belongs to the application. I have not yet found a way to override the application container via modules (although I didn't look much). In any case, it's likely to still take advantage of some form of composite container. Didn't understand the part about
Of course, I would use mock objects. But how would those mocks get into the application container?
Perhaps. This package ships such a module.
I mentioned such a scenario: a case where any service is automatically overridable with data from somewhere else. So, look into the e.g. WP Options container, and if not there - look into the modules' services container.
That 2nd point is also quite a variable thing. Of course, it's possible to go around this with an
Well, yes, because it's the truth. This package is the one that brings together many technologies, and it is the whole point of this package. If you wanna start fresh, and don't need all of this, then why not start from 0? After all, just a WP plugin is simply 1 file with a comment... |
|
I still don't quite understand what are the things that are deal-breaking for you. Without the combination of these different technologies, this package is pretty useless. I disagree that this is a match made in hell. There's also no wrong opinion here. I thought about having a re-usable module loader before starting this package. I started building it, and concluded that it is mostly useless, and does not deserve its own package. If you think you can make one that is useful, I encourage you to try it, and then if it turns out well, I will gladly consider using it here. Is there something else that you really are against having in this package? Maybe, we can do a video call, where I walk you through the code? I think that some of my decision-making will be more clear this way. In any case, thank you for your contribution, and your time. |
The logic that I wanted to combat here was not frequency of use as a whole, but the availability of subsystems on their own.
You're "centralizing" in a boilerplate package. Need I say more?
I have. But not for modules. Like I said before, a module is a preset of existing technologies. I never build modules that need to be shipped with their own dedicated sources. Modules are configuration, not libraries. So I've never had to, and don't think anyone should, need to extract a module into its own package. I'm actually going to stop replying there, because frankly replying to your last message is more meaningful IMO.
I'd just like to see a boilerplate package that merely brings together existing technologies, rather than provide its own. Like I said, there's code in here that is genuinely useful as a dependency for other projects. Moreover, I'd like to see something that promotes good module usage. What I see here goes against what I would consider to be "idiomatic" for the module system that I initially envisioned. That doesn't mean it's bad - it just means I'm biased by my different ideals. Maybe if I shorten my suggestion down it might make more sense:
|
All of the systems are already available on their own, with the exception of a 20-line module loading algorithm, which is dead simple, and out of which maybe 10 lines can really be re-used. This package configures these subsystems to work together.
Oh, I see 😆. Well, maybe the word "centralizing" is the wrong word to use here, but I think you get that I was trying to say "combines" or "aggregates".
Well, I have. Many times. SDK, HTTP client, WP events wrapper, session control, admin notices, and many many more. A lot of those are libraries that additionally ship a module declaration, for ease of use. Some are more about doing something specific in the application itself. This is a very common case, and I wanted to build this into this pacakge Even if it is not common, it is useful to know at a glance which dependencies belong to which modules. Modules definitely can have their own sources, like event handlers for example.
For the most part, this boilerplate simply brings existing technologies together.
|
|
I just realized that I haven't yet committed the changes that use the "WP SDK". Coming up. |
Deps upated
- `me` didn't reflect the actual vendor. - `plugin` is not sufficient, and may lead to conflicts in the future.
# Conflicts: # composer.json
Even if its only 5 lines, the only runtime stuff that is acceptable IMO is a preset entry point that users can edit or replace, like an index.html for web apps. In this case, it would be the plugin main PHP file, in its simplest form.
I didn't, but even so I was still referring to code that is exclusively hosted in this repo.
They can, which is why I'm very vocal against it. But that's a discussion for another thread.
Pretty much, yeah!
My review for this boilerplate is quite simply to de-bloat it. And you've expressed that you are going to do that. Fantastic.
Use it? Probably not. My setups and even general architectures are too different. I'd be tearing out everything except for the developer tools. |
Not sure I understand. We were talking about extracting the loading algorithm. Now you're talking about an entry point. Also, there is an entry point that users can edit or replace: it's the bootstrap function in
And again, I don't understand what that should tell me.
My point is that there's nothing wrong with that. But OK, a different discussion.
I'm really confused. I want to make something useful, including for you. And you're saying that your architecture is too different, which is why you wouldn't use it or contribute. But you are going to promote it? Why? And how would you, if you are not using it? 🤔 |
|
Now available on Packagist as |
If I deem this project to be worth people's time, I can very easily recommend it. Having developed most of the tech here, I have a pretty good idea of how it works. I don't need to be personally using it myself to know that. Like I said, certain things here don't really align with my ideals. But my ideals aren't absolute law, and they shouldn't reflect anything about the project or how it works. The fact is that this project does work. It works for you and it can work for others. I just have my own way of doing things. |
Uh oh!
There was an error while loading. Please reload this page.