-
Notifications
You must be signed in to change notification settings - Fork 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 approach #1
base: master
Are you sure you want to change the base?
Conversation
@mnapoli what do you think of this approach? Is it viable, or would you advise differently? |
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" |
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.
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.
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.
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... |
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.
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.
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.
mm, seems better indeed, will have a look at this!
README.md
Outdated
- run the Application | ||
|
||
## In your ```./public/index.php``` |
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.
Inline code blocks are made with single quotes: "In your ./public/index.php
"
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.
thanks, did not know that...
@mnapoli adopted your suggestion to use a 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 public function __construct(bool $inProduction = false, string $containerClass = 'DI\Container') |
No description provided.