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

Initial approach #1

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Initial approach #1

wants to merge 20 commits into from

Conversation

holtkamp
Copy link
Collaborator

No description provided.

@holtkamp holtkamp self-assigned this Jan 22, 2017
@holtkamp holtkamp requested a review from mnapoli January 22, 2017 11:03
@holtkamp
Copy link
Collaborator Author

holtkamp commented Mar 8, 2017

@mnapoli what do you think of this approach? Is it viable, or would you advise differently?

@mnapoli
Copy link
Member

mnapoli commented Mar 12, 2017

Really sorry for the delay! That looks good, I have a few remarks I'll post them inline in the diff.

composer.json Outdated
"php-di/php-di": "@stable",
"php-di/invoker": "@stable",
"zendframework/zend-expressive": "@stable",
"zendframework/zend-expressive-helpers": "@stable"
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to restrict to major versions to avoid being affected by a BC break in any of those packages.

Random example: the class Zend\Expressive\Container\ApplicationFactory could very well be removed in a new major version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

/* @var \DI\ContainerBuilder $containerBuilder */
$containerBuilder = require __DIR__ . '/../vendor/php-di/zend-expressive-bridge/config/containerBuilder.php';
$inProduction = false; //You probably want to use an environment variable for this...
Copy link
Member

Choose a reason for hiding this comment

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

Here is an idea: instead of requiring the containerBuilder.php file that is in vendor/ there could be instead a class, e.g. ExpressiveContainerBuilder, that extends from ContainerBuilder and auto-add the configuration files of this repository.

The advantage I see is it benefits from autoloading so it avoids the long line of include that goes looking in vendor/ (which is kind of unusual), and also the possibility to get rid of all the if(class_exists('Zend\Expressive\Router\AuraRouter')){ in the config files => the ExpressiveContainerBuilder could have methods dedicated to include configuration for libraries.

Here is an example of what it could look like:

// the $inProduction could be used to enable/disable Twig debug or other debug for example
$containerBuilder = new ExpressiveContainerBuilder($inProduction);
$containerBuilder->registerAuraRouter();
$containerBuilder->registerTwigRenderer();
// All methods of the container builder can still be used of course
$containerBuilder->addDefinitions(/* your own definition files */);

It's a bit less automatic than your current solution but relying on class_exists is risky IMO, it's not because a class is installed in vendor/ that you want to use it in your application. And that way it's much more explicit/easier to understand what's happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mm, seems better indeed, will have a look at this!

README.md Outdated
- run the Application

## In your ```./public/index.php```
Copy link
Member

Choose a reason for hiding this comment

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

Inline code blocks are made with single quotes: "In your ./public/index.php"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, did not know that...

@holtkamp
Copy link
Collaborator Author

holtkamp commented Apr 8, 2017

@mnapoli adopted your suggestion to use a ExpressiveContainerBuilder class, works nice indeed!

Sole point of hesitation is the order of arguments in the constructor:

public function __construct($inProduction = false, $containerClass = 'DI\Container')

will become incompatible with the ContainerBuilder when using more strict types (PHP 7 will be enforced some day):

public function __construct(bool $inProduction = false, string $containerClass = 'DI\Container')

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

Successfully merging this pull request may close these issues.

2 participants