Issue #57: Distinguish files depending on dev environment.

[FlorentTorregrosa]

No description provided.

[greg-1-anderson]

Maybe includes-dev (like require-dev)?

[FlorentTorregrosa]

Hello,

Thanks for the review. I have pushed a commit to take your suggestion into account.

[greg-1-anderson]

LGTM. 👍

@derhasi @webflo Any feedback?

[derhasi]

Looks okay to me.
One thing, I am currently not sure about: Maybe we can even put sites/example.settings.local.php and sites/example.sites.php in dev mode, but I don't know if this would break Drupal in any way.

[webflo]

I am not convinced of this feature, because initially we told people to commit the files from drupal-scaffold to their repository. drupal-scaffold was meant to be run on the initial creation of a project (e.g. composer create-project) and on updates of drupal/core to have always the proper config in place.

I won't block the PR, its just for the discussion.

[greg-1-anderson]

Folks could .gitignore the dev files and run drupal-scaffold to get them in a dev environment, so this still seems useful, even if committing the scaffold files.

[FlorentTorregrosa]

Hello,

Thanks for the discussion, here are thoughts and opinion.

@derhasi, I intend to put sites/example.settings.local.php in excludes on my projects, I already have a sites/development.settings.php (better name in my opinion) https://github.com/Drupal-FR/socle-drupalcampfr/tree/8.x-1.x/www/sites.

For sites/example.sites.php, I have no idea. I remember having problem if it was not here, I scanned Drupal core and Drush, there are references to this file but I see nothing that would prevent an installation.

@webflo, even if Drupal scaffold is thought to be used with the files versionned, I personnally ignore them as they are "generated" (obtained from "contrib"), except files I have modified like settings.php.

Before I used a custom script to do what drupal-scaffold do and this weekend I decided to use it as it is one less script to maintain and to stick more with the best practice of the community. Drupal-FR/socle-drupalcampfr@08b6606

[webflo]

Ok, works for me. Can we add a test for it?

[FlorentTorregrosa]

Hello,

I have written a test case.

I wanted to extend the PluginTest class to avoid duplicate code but I got the following error in that case PHP Fatal error: Class 'DrupalComposer\DrupalScaffold\Tests\PluginTest' not found in drupal-scaffold/tests/HandlerTest.php on line 13

Also may I suggest to add prefer-stable' => true, for the composerJSONDefaults method to avoid having to clone almost all dependencies and to have quicker tests.

Thanks for the review.

[FlorentTorregrosa]

Hello,

Is the last patch ok for you?

[greg-1-anderson]

This is also called from (Plugin.php)[https://github.com/drupal-composer/drupal-scaffold/blob/2.3.0/src/Plugin.php#L77).

Rather than risking changing the API of this method every time we have an enhancement to the state we respond to, it would probably be better to pass the $event in here, and let downloadScaffold() access $event->isDevMode() itself.

[greg-1-anderson]

This looks great. Try having PluginTest and HandlerTest extend the same base class, and push duplicate code down there.

[FlorentTorregrosa]

Hello,

Both proposed changes has been addressed.

Is it ok now?

Edit: And thanks @greg-1-anderson for the review at DrupalCon Vienna!

[FlorentTorregrosa]

Hello,

I have rebased my changes to followup the last code changes in drupal-scaffold.

Would it be possible to have a review please?

[FlorentTorregrosa]

Hello,

I have rebased my pull request and made some adjustments following the recent commits.

Would it be possible to merge it please?

[FlorentTorregrosa]

Hello,

Now that #88 is ok and will be merged soon, I have rebased this PR to fix conflicts.

Will it be possible to have it reviewed one more/last time and hopefully merged please? :)

[RaphTbm]

Hi! There is some limitation with the current implementation, it doesn't allow to scaffold dev files in a different folder (if my drupal folder is in /web using drupal-project for example).
I would suggest to create an "initial-dev" as you have done for "includes" with "includes-dev"?