Skip to content
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

Alternate Proposal for 0.2.x #17

Open
wants to merge 32 commits into
base: 0.3.x
Choose a base branch
from
Open

Alternate Proposal for 0.2.x #17

wants to merge 32 commits into from

Conversation

mecha
Copy link
Member

@mecha mecha commented Nov 14, 2019

Alternate Proposal for 0.2

This branch is an alternate version of the original 0.2 branch, with the following prominent changes.

Changes

Exceptions

This branch provides exception concrete classes instead of interfaces. Exception interfaces serve zero utility in practice. Feel free to try and change my mind.

This branch also removes the ModuleSetupException. The reason for this will become evident in the next paragraph.

Interfaces

Removed the ModuleInterface::setup() method (and therefore also the ModuleSetupException). In its place, the below methods were added to the interface:

  • getFactories() that returns an array of FactoryInterface
  • getExtensions() that returns an array of ExtensionInterface

As such, the below interfaces were added:

  • ServiceInterface
  • FactoryInterface which extends ServiceInterface
  • ExtensionInterface which extends ServiceInterface

This change effectively decouples this package from the (still experimental) service provider spec. The reasoning for this can be found in the next section.

Helper Implementations

There are a number of helper service implementations included in this branch. I firmly believe that a module system package would not be complete without at least 2 of these implementations: Factory and Extension. The remainder could be moved into a separate package. But for now they will exist here, in this branch, to help keep this proposal centralized.

Motivation & Reasoning

Decoupling from SP spec

Firstly, there is no guarantee that the service provider spec will ever be accepted. If it "dies", we'd need to substitute ServiceProviderInterface with an equivalent interface, possibly created by us.

Static analysis and module manipulation

Secondly, the benefits gained from being able to return FactoryInterface and ExtensionInterface arrays, instead of just callable arrays, outweigh the negligible gain from incorporating the service provider spec. Especially since no interoperability is lost (see the 3rd point).

By requiring instances of these new interfaces, each service (be it a factory or extension) will be able to provide its dependency list. This allows for the creation of static analysis tools, optimizers, caching mechanisms, container compilers and so on.

Furthermore, because the services themselves become decoupled from service keys, those service keys could theoretically be changed without breaking any of the services. This would allow the application to manipulate modules before incorporating them, giving back some control to the application. This, in turn, would allow the same module be included in the application multiple times (a.k.a. "multiboxing").

Compatibility is retained

Lastly, it's still compatible with service providers.

new ServiceProvider($module->getFactories(), $module->getExtensions());

This is thanks to the fact that factory and extension implementations are invocables that have the expected function signature. To any standards-compliant service provider and DI container implementation out there, the factories and extensions given by a module will appear to be nothing more than just functions.

@mecha mecha added this to the v0.2 milestone Nov 14, 2019
@mecha mecha self-assigned this Nov 14, 2019
@mecha
Copy link
Member Author

mecha commented Jan 10, 2020

I've come to the conclusion that the concrete helpers can safely exist in their own package. Since their use is mostly private to module implementations, module authors could opt-into using them by installing that package.

The only thing that I'd take from this draft PR is the concrete exceptions. The rest can be thrown away.

@XedinUnknown
Copy link
Member

@mecha would you like to bring this PR into shape, so that we can merge, and accept the new standard?

@mecha mecha changed the title Proposal for 0.2.x Alternate Proposal for 0.2.x Feb 10, 2020
@mecha mecha marked this pull request as ready for review February 10, 2020 17:27
@XedinUnknown
Copy link
Member

I did not go in detail over each class here. But here are some points. TL;DR: I like some of the changes, but I feel that the majority of them do not belong in this package.

  1. Probably the most radical change, and the change that I dislike the most, is that with the substitution of setup() and run() with getFactories()andgetExtensions()`, the module becomes in essence a glorified Service Provider, and that defeats the point of the module standard. The new methods, no doubt, could have a benefit over a service provider, but then they belong in an extension (or substitute) of the Service Provider standard, and not here.

  2. I would like this to remain an interface package, akin to PSR standard packages, and therefore there shouldn't be actual implementations here.

  3. The benefit in throwing interfaces rather than concretes is the same as the benefit of having interfaces over concretes in any other case. However, I would take an alternative approach, and just declare that they throw generic Exception. What do you think about that? Also, see point 2.

  4. It's not the first time I look at these helpers, and they look very useful. But see point 2.

In general, my idea of an optimal approach from this point is as follows.

Let's carry on with the original 0.2 spec, get it ready (see point 3), merge, and release. If you feel that Service Provider needs an overhaul, then let's approach FIG (I'll support you there), and if they don't want it, let's create an extension: as you said, not really a conflict here. I would like to have modules be ale to do stuff, not just provide stuff, because the idea of this package is that of a generic module. IMHO, what we already have in 0.2 is a great solution, it is small and simple, very generic, and has worked for you and me for some time now.

@XedinUnknown XedinUnknown mentioned this pull request Feb 11, 2020
@mecha
Copy link
Member Author

mecha commented Feb 11, 2020

My replies here are intentionally out-of-order, to be more coherent.


I would like to have modules be ale to do stuff, not just provide stuff

You seem to be implying that my proposal does not let modules run(). So I'm confused here.


the module becomes in essence a glorified Service Provider, and that defeats the point of the module standard

How so? The point of a module system is to allow an application to be segmented into individual, ideally agnostic, parts. I don't see how a module's ability to provide services hinders that, especially since the original 0.2 spec does the same thing (but through a SP that acts as an intermediate).


The benefit in throwing interfaces rather than concretes is the same as the benefit of having interfaces over concretes in any other case

Yes, consuming an interface is better than consuming a concrete. But if the concretes are all the same, then there's no reason to have an interface. As a consumer of this package, ModuleExceptionInterface has no value to me. When I catch exceptions, I don't care for alternate implementations. And when I throw, I need a concrete. So IMO the interfaces only exist for the sake of SOLID, which I don't deem to be a good thing. Bloat is bloat, no matter how shiny it is.


I would like this to remain an interface package, akin to PSR standard packages, and therefore there shouldn't be actual implementations here.

This makes me sad. Perhaps because I want a module system, while you want a module standard. I have no interest in the latter. I see no point in creating an interface standard for something new. That's something that FIG do, and they do it with purpose: to unify frameworks.

Yet we do not consult with frameworks. We're forging a new solution, not standardizing existing ones. For this reason, making a decent module system implementation is important for me. If it becomes de facto, then for all intents and purposes it becomes "standard".


If this proposal does not acquire your approval, I will probably host it myself at mecha/modular-php or something. Not out of pride or anything like that. But I have actual work-related requirements for a module system that lets me box, manipulate and statically analyze modules, and the current v0.2 spec is inferior. So I might have no choice but to fork.

@XedinUnknown
Copy link
Member

Yep, I misunderstood what you wrote, thinking that modules no longer have the run(). But then, why change the ModuleInterface to receive getFactories() and getExtensions() when it already has a setup() method which returns something that has those methods?

I guess my biggest point and feeling was that with the current 0.2 spec we can build on top of what already exists, rather than re-define it. Which makes me quite confused as to why you require either adapting the new standard or forking: why would you (or we) not be able to build a module system on top of a module standard? The system you want appears to be fully compatible to the standard already planned. So, what's the problem?

About FIG. Yes, it stands for Framework Interop Group. And their processes involve consulting frameworks. But their final products have nothing to do with any particular frameworks. They are just simply standards that anyone can use. Which is what I wanted this package to also be.

So, why don't we accept the original 0.2 spec, and then open a dhii/modular-php (or something), and put the implementation in this branch there?

@XedinUnknown
Copy link
Member

About Exceptions. What you wrote seems to contradict itself to me.

But if the concretes are all the same, then there's no reason to have an interface.

You don't know if the concretes are all the same. That's the point.

As a consumer of this package, ModuleExceptionInterface has no value to me. When I catch exceptions, I don't care for alternate implementations.

Exactly, you don't care for alternate implementations. By catching ModuleExceptionInterface, any implementations are completely transparent to you. Catching ModuleExceptionInterface is the same effort as catching ModuleException concrete, except for the word Interface. It's the same thing, but SOLID lets you be more flexible. That's also the point.

And when I throw, I need a concrete.

When you need to throw a concrete, then throw a concrete. If we go ahead with a module system implementation package, then that will be the package where all the concretes live. What seems to be the problem?


Also, what did you think of my suggestion to drop proprietary exceptions and just use native ones?

@mecha
Copy link
Member Author

mecha commented Feb 12, 2020

Yep, I misunderstood what you wrote, thinking that modules no longer have the run(). But then, why change the ModuleInterface to receive getFactories() and getExtensions() when it already has a setup() method which returns something that has those methods?

Looks like you completely missed the point of these changes then. I cannot iterate a service provider's factories and for each factory get its dependencies. Because service provider promises callable[] for factories and extensions. These new methods promise Factory[] and Extension[] respectively.

Also, what did you think of my suggestion to drop proprietary exceptions and just use native ones?

I agree. The thing is, anything can throw an Exception (or RuntimeException for that matter). So to me, your proposal is basically don't mention exceptions anywhere. Which is good for me.

So, why don't we accept the original 0.2 spec, and then open a dhii/modular-php (or something), and put the implementation in this branch there?

I can't accept the original 0.2 spec because it doesn't work for me. I need this version, or another version of the original that lets me do what this version does.

@mecha
Copy link
Member Author

mecha commented Feb 12, 2020

I've been doing some thinking, and I've identified what I feel strongly about and what I feel we could agree on.

First, de stronk:

  1. Coupling a module standard to the service provider standard is big no-no for me.
  2. Services (both factories and extensions) should be able to provide their dependencies (in string key form)
  3. A module standard should provide at least 2 concretes: Factory and Extension
  4. A document must be created that clearly states how modules should be built. For instance, no prefixing of keys and no references to other module's services inside factories.

Reasoning

When I depend on a standard, be it psr/container, psr/log, or whatever, I expect to have everything I need to build an implementation of the thing that the standard represent: a container or a logger respectively.

Therefore, when I depend on a module standard, I expect to have everything I need to build a module implementation. This means having Factory and Extension implementations ready to go. This in turn means ditching service provider.

Of course, what is the benefit of having a module standard be separated from an actual module system? The ability to only depend on the things required to create a module. In other words, the ability to create a package that provides a module, but not a modular app.

The implication here is that modules MUST be developed agnostic of each other in order to be standards compliant. This means clearly defining the standard in textual form. Not only that, but applications must be able to manipulate 3rd party modules to make them fit the application.

All of this culminates in the above fours points, which revolve around services being able to provide their dependencies.


The rest:

  1. We keep the module standard and the module system in separate packages.
  2. The module system must be a complete package. Applications that depend on it should be able to simply install it and go. No need to depend on anything else. This includes the module standard package.
  3. A third package can be created for modules (as opposed to the module system, which is targetted for applications). This package must also be complete in the same way, and would provide the service helpers that I proposed in this branch.

The end result is: Module packages will need to require the package mentioned in point 7, while applications will need to require the package mentioned in point 6. If a package contains both an application and its modules, then it might need to require both. The key point here is that each case only has 1 package dependency.

@XedinUnknown XedinUnknown changed the base branch from 0.2.x to develop April 10, 2020 14:13
@XedinUnknown
Copy link
Member

The arguments you listed under "The rest" are all good for me 👍 . The point I strongly disagree with is the coupling to the Service Provider standard. This is an interop standard developed by FIG members with the intention of becoming a PSR. I don't see why we should not follow that.

As for the other points, I agree in general, and the details are debateable. AFAIK I remember that we had agreed to add some of these things later, on top of the existing module standard. I'd be happy if we do that. I know you've already got a bunch of cool implementations ready :)

@mecha
Copy link
Member Author

mecha commented Apr 11, 2020

The point I strongly disagree with is the coupling to the Service Provider standard. This is an interop standard developed by FIG members with the intention of becoming a PSR. I don't see why we should not follow that.

Here are all the reasons (that I can remember) that I've given you over the past few months:

  • It's still experimental and may never be a PSR
  • Implementing the SP interface inhibits the standard. Without SP:
    • A module can be serialized/compiled
    • A module can be multi-boxed
    • An app can assign prefixes to modules
    • We can build tools for module static analysis
  • Correctness: a module is not a service provider, but a slice of an application
  • We can still be compatible with SP without implementing the interface (via combo-class or decorator)

@XedinUnknown
Copy link
Member

Discussion on SP has been restarted in container-interop/service-provider#51. @mecha, if you still want to implement these changes, consider changing the target to 0.3.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants