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

SimpleApplicationFactory #43

Closed
wants to merge 4 commits into from

Conversation

asgrim
Copy link
Member

@asgrim asgrim commented Jun 4, 2020

Q A
Documentation yes
Bugfix no
BC Break no
New Feature yes
RFC yes
QA no

Description

This PR adds a new factory \Mezzio\Container\SimpleApplicationFactory, and documentation alongside the feature. As discussed in TSC chat, my aim is to simplify creation of an Application instance, such that it is comparable with other similar frameworks. In order to do so, of course we need to make some assumptions here, which I've tried to list.

Note: I don't consider this PR ready to merge; I'd like to open a discussion on how flexible we want to make this and what the best way is; but we should carefully consider each trade-off for making configurable vs simplicity out the box.

Original basis for this: gist 1 gist 2.

@asgrim asgrim force-pushed the feature/simple-app-factory branch 2 times, most recently from 48e8681 to 9912c15 Compare June 4, 2020 21:51
@Xerkus
Copy link
Member

Xerkus commented Jun 4, 2020

@asgrim may be it would make more sense as another lightweight skeleton package instead?

src/Container/SimpleApplicationFactory.php Outdated Show resolved Hide resolved
$app = SimpleApplicationFactory::create($container->reveal(), $router->reveal());

// Check we can perform something simple like adding a route
$router->addRoute(Argument::type(Route::class))->shouldBeCalled();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if a full dispatch wouldn't be a better scenario here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible, but a LOT of work, since we've made many things "unmockable" here (intentionally, it's a factory that makes assumptions); so things like \Laminas\Diactoros\ServerRequestFactory::fromGlobals become a bit more tricky. Not sure it's worth the effort here?

Copy link
Contributor

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few nitpicks.

I tend to agree with @froschdesign and @Ocramius that "Minimal" would be better here. We may consider it "simple", but somebody not as familiar with interfaces, containers, and whatnot might not!

One more point I'd make: the container is used internally primarily to pull middleware and handlers that have been piped/routed by name instead of by _instance. With a minimal setup like this, is it terribly useful? Could we not just provide an anonymous PSR-11 implementation?

Additionally, we may want to consider, in the future, adding tooling to migrate a minimal app to a skeleton. But you can ignore that comment for now. :)

@boesing
Copy link
Member

boesing commented Jun 5, 2020

Should we considering some "minimal" application factory around https://github.com/php-http/discovery?

I dont like the magic of php-http/discovery but as of this PR, I would rather use that library instead of depending on assumptions that laminas-diactoros is installed which might not be true.

As @Xerkus stated, this could be done in a pretty simple component which just adds the new MinimalApplicationFactory and has a dependency on php-http/discovery.

Thus, there is no need for all that other stuff (but, yes, there would be a new component and its not as simple as this PR anymore).


Sadly, there is no "magic" library which can autodiscover containers. Thats where the upcoming laminas-container-contracts package might become handy? @weierophinney thoughts?

asgrim added a commit to asgrim/mezzio that referenced this pull request Jun 10, 2020
Suggestions from @weierophinney in PR mezzio#43

Co-authored-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
asgrim and others added 4 commits June 10, 2020 20:08
… simpler

Signed-off-by: James Titcumb <james@asgrim.com>
Signed-off-by: James Titcumb <james@asgrim.com>
Suggestions from @weierophinney in PR mezzio#43

Co-authored-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: James Titcumb <james@asgrim.com>
Signed-off-by: James Titcumb <james@asgrim.com>
@asgrim asgrim force-pushed the feature/simple-app-factory branch from 3655a6c to d6bad8f Compare June 10, 2020 19:08
@asgrim
Copy link
Member Author

asgrim commented Jun 10, 2020

One more point I'd make: the container is used internally primarily to pull middleware and handlers that have been piped/routed by name instead of by instance. With a minimal setup like this, is it terribly useful? Could we not just provide an anonymous PSR-11 implementation?

I'm unclear what you mean here @weierophinney ? In my clients application where I'm using this (or rather, the basis of this), I use the container to assign to the pipeline, e.g.:

$container = new ServiceManager();
$container->setFactory(MyMiddleware::class, /** stuff */);
$container->setFactory(MyHandler::class, /** stuff */);

// -- later .... .... -

$app->pipe(MyMiddleware::class);
$app->get('/best-url', MyHandler::class, MyHandler::class);

If I understand what you're saying, this wouldn't be possible I think?

@asgrim
Copy link
Member Author

asgrim commented Jun 10, 2020

I dont like the magic of php-http/discovery but as of this PR, I would rather use that library instead of depending on assumptions that laminas-diactoros is installed which might not be true.

It is indeed an assumption; but if Diactoros is not installed, it'll be fairly obvious why, since autoloading will complain about missing dependencies in Laminas\Diactoros. It is a trade-off indeed, hence why I put to others to consider :)

@weierophinney
Copy link
Contributor

I'm unclear what you mean here @weierophinney ? In my clients application where I'm using this (or rather, the basis of this), I use the container to assign to the pipeline, e.g.:

Okay - I wasn't clear that you intended to allow that usage, from the examples you provided (which all involved creating an empty container)! I'm fine with leaving as-is, then.

@weierophinney
Copy link
Contributor

I dont like the magic of php-http/discovery but as of this PR, I would rather use that library instead of depending on assumptions that laminas-diactoros is installed which might not be true.

It is indeed an assumption; but if Diactoros is not installed, it'll be fairly obvious why, since autoloading will complain about missing dependencies in Laminas\Diactoros. It is a trade-off indeed, hence why I put to others to consider :)

I think the whole idea of this is to be as opinionated as possible, to provide a "works-out-of-the-box" solution.

With that said: I wonder if this might be an argument for having a separate package for this factory? It could then explicitly list those requirements, and users would do something like:

$ composer require mezzio/mezzio-minimal
$ ./vendor/bin/laminas mezzio:minimal:create index.php # creates a template for a minimal install in the index.php file
$ vim index.php # edit the file to add routes
$ ./vendor/bin/laminas mezzio:minimal:serve -p 8080 -h 127.0.0.1 # fire it up

Thoughts?

@asgrim
Copy link
Member Author

asgrim commented Jun 10, 2020

I dont like the magic of php-http/discovery but as of this PR, I would rather use that library instead of depending on assumptions that laminas-diactoros is installed which might not be true.

It is indeed an assumption; but if Diactoros is not installed, it'll be fairly obvious why, since autoloading will complain about missing dependencies in Laminas\Diactoros. It is a trade-off indeed, hence why I put to others to consider :)

I think the whole idea of this is to be as opinionated as possible, to provide a "works-out-of-the-box" solution.

With that said: I wonder if this might be an argument for having a separate package for this factory? It could then explicitly list those requirements, and users would do something like:

$ composer require mezzio/mezzio-minimal
$ ./vendor/bin/laminas mezzio:minimal:create index.php # creates a template for a minimal install in the index.php file
$ vim index.php # edit the file to add routes
$ ./vendor/bin/laminas mezzio:minimal:serve -p 8080 -h 127.0.0.1 # fire it up

Thoughts?

I'd be interested in taking that on, sure. I'll put together a prototype repo on my own GitHub and loop back here. Great ideas folks, thank you all.

@asgrim
Copy link
Member Author

asgrim commented Jun 18, 2020

OK folks; I've moved this to https://github.com/asgrim/mini-mezzio now 👍

Thoughts/issues/suggestions/PRs are welcome there 😁

Closing this now.

@asgrim asgrim closed this Jun 18, 2020
@asgrim asgrim deleted the feature/simple-app-factory branch June 18, 2020 22:21
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.

6 participants