Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Allow choosing minimal, flat, or modular structure during installation #124

Merged

Conversation

weierophinney
Copy link
Member

Per commentary on #54, this patch updates the installer to allow for either of:

  • Minimal installation (no default middleware, templates, or assets)
  • Flat structure (what was shipped in 1.0; remains the default)
  • Modular structure (makes the 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:

  • Run in a separate process. (This was necessary to prevent caching issues.)
  • Re-create the project source files (config/, public/, src/, test/) under a new directory under the system temporary directory.
  • Notify the installer of the new project root, and change working directories to that location.
  • Clean up after verification.

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.

- 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
@weierophinney weierophinney added this to the 1.1.0 milestone Jan 16, 2017
- Evidently, `templates/` does not exist by default.
Copy link
Member

@geerteltink geerteltink left a 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 executes post-create-project-cmd: @development-enable in flat mode. After running composer update the composer development-enable is working.

  • mikey179/vfsstreamis 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. Since sys_get_temp_dir() we could rewrite the CopyFileTest and remove vfsstream completely.

  • In ExpressiveInstallerTest\InstallerTestCase the line use 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/';
Copy link
Member

@geerteltink geerteltink Jan 17, 2017

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.

Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member Author

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',
Copy link
Member

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

Copy link
Member

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.

Copy link
Member Author

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',
Copy link
Member

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',
Copy link
Member

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',
Copy link
Member

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.

Copy link
Member Author

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',
Copy link
Member

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.

Copy link
Member Author

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.

@weierophinney
Copy link
Member Author

@xtreamwayz

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 executes post-create-project-cmd: @development-enable in flat mode. After running composer update the composer development-enable is working.

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 App module during installation, instead of having it enabled in the configuration by default; that would allow development mode to trigger immediately. I'll give that a try today.

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.

I noticed this as well when running create-project — you'll note that you get prompted to install the config providers for zend-validator and one other ZF component when installing, and these are both only because the components are in the original require-dev section, prior to rewriting. So it's not isolated to vfsStream.

That said, I'll still see about rewriting the CopyFileTest to remove that dependency; shouldn't be hard.

In ExpressiveInstallerTest\InstallerTestCase the line use org\bovigo\vfs\vfsStream; can be removed as it's not used.

Will make that change today.

Thanks for all the feedback!

@geerteltink
Copy link
Member

geerteltink commented Jan 17, 2017

A fix for the post-create-project-cmd: @development-enable execution issue:

Change (towards the end of OptionalPackages::install):

    // Set required packages
    $rootPackage->setRequires(self::$composerRequires);
    $rootPackage->setDevRequires(self::$composerDevRequires);

    // Set stability flags
    $rootPackage->setStabilityFlags(self::$stabilityFlags);

with:

    // Update root package
    $rootPackage->setRequires(self::$composerRequires);
    $rootPackage->setDevRequires(self::$composerDevRequires);
    $rootPackage->setStabilityFlags(self::$stabilityFlags);
    $rootPackage->setAutoload(self::$composerDefinition['autoload']);
    $rootPackage->setDevAutoload(self::$composerDefinition['autoload-dev']);

Second fix:

Turns out removing dependencies are case sensitive! Change 'mikey179/vfsStream' to 'mikey179/vfsstream' in the OptionalPackages::$devDependencies array.

I noticed this as well when running create-project — you'll note that you get prompted to install the config providers for zend-validator and one other ZF component when installing, and these are both only because the components are in the original require-dev section, prior to rewriting.

No, they are requirements for zend-expressive-zendrouter:

 zendframework/zend-expressive-zendrouter 2.0.0
 ├──zendframework/zend-expressive-router ^2.0
 ├──zendframework/zend-psr7bridge ^0.2.2
 │  └──zendframework/zend-http ^2.5
 │     ├──zendframework/zend-uri ^2.5
 │     │  └──zendframework/zend-validator ^2.5
 │     └──zendframework/zend-validator ^2.5
 └──zendframework/zend-router ^3.0
    ├──zendframework/zend-http ^2.5
    │  ├──zendframework/zend-uri ^2.5
    │  │  └──zendframework/zend-validator ^2.5
    │  └──zendframework/zend-validator ^2.5

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.

@weierophinney
Copy link
Member Author

A fix for the post-create-project-cmd: @development-enable execution issue:

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
@weierophinney weierophinney force-pushed the feature/minimal-modular-flat branch from 7e8488d to 583720a Compare January 17, 2017 19:49
@weierophinney
Copy link
Member Author

That said, I'll still see about rewriting the CopyFileTest to remove that dependency; shouldn't be hard.

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 glob(). CopyFileTest does not require that.

Class is not intended to be extended, so we can remove methods we do not use.
@weierophinney weierophinney merged commit 1cb26dd into zendframework:develop Jan 17, 2017
weierophinney added a commit that referenced this pull request Jan 17, 2017
@weierophinney weierophinney deleted the feature/minimal-modular-flat branch January 17, 2017 20:27
@weierophinney weierophinney modified the milestones: 1.1.0, 2.0.0 Jan 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants