-
Notifications
You must be signed in to change notification settings - Fork 90
Allow choosing minimal, flat, or modular structure during installation #124
Allow choosing minimal, flat, or modular structure during installation #124
Conversation
- We can also fully remove the src/App and test/AppTest directories. - Doing so requires we conditionally remove files from the src/ tree during cleanup, however, as they may not be present.
Modifies the first question to be a multi-choice selection, instead of a y/n selection: - 1: minimal install - 2: flat install (default) - 3: modular install (recommended) Also updates select test cases in the test suite to copy the project files into a new directory in the system temporary tree, running installation from that location to verify that each of the following work as expected: - Containers - Routers - Template renderers - Error handlers
- Evidently, `templates/` does not exist by default.
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.
Overall I really like the changes.
I have tested the script locally with this: https://gist.github.com/xtreamwayz/7590e55f95e98daf8d930b64cd8e4b69
Issues I found:
-
In flat mode only I get this:
Cannot read config from App\ConfigProvider - class cannot be loaded.
This is only in flat mode (minimal and modular works as expected). It goes wrong when composer executespost-create-project-cmd: @development-enable
in flat mode. After runningcomposer update
thecomposer development-enable
is working. -
mikey179/vfsstream
is installed while it's not in composer.json ??? AFAIK dev dependencies of included packages are not installed. Yet it is installed. We have had this a long time ago but I fixed that by updating the stability flags. It looks like this code hasn't changed so maybe there is something else we missed,... or it's a composer bug. Sincesys_get_temp_dir()
we could rewrite theCopyFileTest
and removevfsstream
completely. -
In
ExpressiveInstallerTest\InstallerTestCase
the lineuse org\bovigo\vfs\vfsStream;
can be removed as it's not used.
self::recursiveRmdir(self::$projectRoot . '/src/App/src'); | ||
|
||
// Re-define autoloading rules | ||
self::$composerDefinition['autoload']['psr-4']['App\\'] = 'src/App/'; |
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.
Flat project, doesn't that mean not having the App dir? I mean the Actions would be located in /src/Action
and not /src/App/Action
. You would still have an App module in src
.
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 current skeleton defines the tree:
src/
App/
Action/
HomePageAction.php
HomePageActionFactory.php
PingAction.php
As such, "flat" is meant to mimic that structure; it's essentially marking src/
as a PSR-0 root directory, if that makes sense.
@@ -472,8 +537,8 @@ public static function removeLinesContainingStrings(array $entries, $content) | |||
/** | |||
* Ask if the user would like a minimal install. | |||
* | |||
* @deprecated since 1.1.0, and no longer used internally |
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.
Why deprecated? For a normal project I understand why public methods are deprecated and later removed. However this installer self destructs itself after it's ready. So in theory this installer never can get extended and its public methods can never get called after installation.
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.
Good point; I'll remove it.
], | ||
|
||
'require-dev' => [ | ||
'filp/whoops' | ||
], | ||
|
||
'application' => [ | ||
'flat' => [ | ||
'/Resources/src/ConfigProvider.flat.php' => '/src/App/ConfigProvider.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.
These absolute paths are confusing
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 can understand why they are confusing. It's caused by the copyFile function:
copy(__DIR__ . $source, $projectRoot . $target);
It uses the relative dir for the source Vs the project root for the target. If we change this so the source is using the project root as well, it becomes more powerful and it can be used to copy files from anywhere if needed.
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.
@Ocramius They're relative to the project root. By having the leading slash, it simplifies logic internally (we don't need to worry about concatenating a slash to the installer/project roots and/or between the installer/project roots and these paths). I'm willing to change them, but it risks adding complexity.
'/Resources/src/ConfigProvider.flat.php' => '/src/App/ConfigProvider.php', | ||
], | ||
'modular' => [ | ||
'/Resources/src/ConfigProvider.modular.php' => '/src/App/src/ConfigProvider.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.
These absolute paths are confusing
'/Resources/templates/plates-layout.phtml' => '/src/App/templates/layout/default.phtml', | ||
'/Resources/templates/plates-home-page.phtml' => '/src/App/templates/app/home-page.phtml', | ||
'/Resources/templates/plates-404.phtml' => [ | ||
'flat' => '/templates/error/404.phtml', |
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.
Same as above
'/Resources/templates/zend-view-home-page.phtml' => [ | ||
'flat' => '/templates/app/home-page.phtml', | ||
'modular' => '/src/App/templates/app/home-page.phtml', | ||
], | ||
], | ||
'minimal-files' => [ | ||
'/Resources/config/templates-zend-view.php' => '/config/autoload/templates.global.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.
Can we remove this minimal-files
options and only have the copy-files
key?
'copy-files' => [
'/Resources/config/templates-zend-view.php' => '/config/autoload/templates.global.php',
'/Resources/templates/zend-view-404.phtml' => [
'flat' => '/templates/error/404.phtml',
'modular' => '/src/App/templates/error/404.phtml',
],
'/Resources/templates/zend-view-error.phtml' => [
'flat' => '/templates/error/error.phtml',
'minimal' => '/src/App/templates/error/error.phtml',
],
],
If the source key is a single string it's always copied. If it's an array if will only be copied for flat
, modular
, minimal
. This makes the config a bit easier and I'm thinking about a possible future disco integration here.
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 thought of that as I started doing this, but was having trouble figuring out a clean way to make it happen. I'll see what I can come up with today.
'/Resources/src/ConfigProvider.flat.php' => '/src/App/ConfigProvider.php', | ||
], | ||
'modular' => [ | ||
'/Resources/src/ConfigProvider.modular.php' => '/src/App/src/ConfigProvider.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.
Use something like this?
'application' => [
'copy-files' => [
'/Resources/src/ConfigProvider.flat.php' => [
'flat' => '/src/App/ConfigProvider.php',
],
'/Resources/src/ConfigProvider.modular.php' => [
'modular' => '/src/App/src/ConfigProvider.php',
],
],
],
It's more inline with other questions.
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 problem with that is that we only want one of those files copied in — either the flat or the modular version becomes the ConfigProvider.php
file, and the target is based on the install type. (The two source files are different, as they present different template path configuration.)
I'll see if I can come up with another solution, though.
Ugh, this may be a chicken-egg problem. We have an autoloader setup by default for the modular structure currently, and then, during install, rewrite it to the flat structure. What I'm thinking is that we may need to enable the
I noticed this as well when running That said, I'll still see about rewriting the
Will make that change today. Thanks for all the feedback! |
A fix for the Change (towards the end of OptionalPackages::install):
with:
Second fix: Turns out removing dependencies are case sensitive! Change
No, they are requirements for zend-expressive-zendrouter:
As said on IRC, the prompt is only there if you select zend-expressive-zendrouter as the router. All other packages don't trigger installation of those packages or that prompt. |
Thanks - made those changes (and the others you suggested) and pushed. Working on the other feedback now. |
- Fixes issue with post-create-project-cmd invocation - Fixes issue with non-removal of vfsstream package - Fixes issue with registration of require-dev ZF component packages that are removed by installer
No longer used.
7e8488d
to
583720a
Compare
Actually, on review of this test case, a mock filesystem is a better plan. Mocking didn't work with the other tests, as they needed to aggregate the configuration, which did not work with vfsStream due to inability to work with |
Class is not intended to be extended, so we can remove methods we do not use.
Per commentary on #54, this patch updates the installer to allow for either of:
App
namespace into a "modular" structure; recommended)This required rewriting a fair number of tests, as the location of the
App\ConfigProvider
will now vary based on the selection, as will templates, and we needed a way to ensure that PHPUnit does not cache the locations between tests.As such, a number of the tests now do the following:
config/
,public/
,src/
,test/
) under a new directory under the system temporary directory.I've also edited all distribution configuration files to ensure they follow the same guidelines and structure (importing classes used, etc.) and to ensure dependencies that are no longer needed are not present.