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

Issue #57: Distinguish files depending on dev environment. #58

Open
wants to merge 9 commits into
base: master
from

Conversation

@FlorentTorregrosa
Copy link
Contributor

commented Feb 11, 2017

No description provided.

README.md Outdated
@@ -38,6 +38,9 @@ of your root `composer.json`.
"includes": [
"sites/default/example.settings.my.php"
],
"dev": [

This comment has been minimized.

Copy link
@greg-1-anderson

greg-1-anderson Feb 11, 2017

Collaborator

Maybe includes-dev (like require-dev)?

FlorentTorregrosa added a commit to FlorentTorregrosa/drupal-scaffold that referenced this pull request Feb 12, 2017

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2017

Hello,

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

@greg-1-anderson

This comment has been minimized.

Copy link
Collaborator

commented Feb 12, 2017

LGTM. 👍

@derhasi @webflo Any feedback?

@derhasi

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

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

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

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

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

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

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2017

Hello,

Is the last patch ok for you?

*/
public function downloadScaffold() {
public function downloadScaffold($dev = FALSE) {

This comment has been minimized.

Copy link
@greg-1-anderson

greg-1-anderson Sep 29, 2017

Collaborator

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 comment has been minimized.

Copy link
Collaborator

commented Sep 29, 2017

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

@FlorentTorregrosa FlorentTorregrosa force-pushed the FlorentTorregrosa:master branch from b012a24 to 2b4d09a Oct 7, 2017

FlorentTorregrosa added a commit to FlorentTorregrosa/drupal-scaffold that referenced this pull request Oct 7, 2017

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

Hello,

Both proposed changes has been addressed.

Is it ok now?

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

FlorentTorregrosa added a commit to FlorentTorregrosa/drupal-scaffold that referenced this pull request Dec 9, 2017

@FlorentTorregrosa FlorentTorregrosa force-pushed the FlorentTorregrosa:master branch from 5364fbd to 21f28fb Dec 9, 2017

FlorentTorregrosa added a commit to FlorentTorregrosa/drupal-scaffold that referenced this pull request Dec 9, 2017

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2017

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 added a commit to FlorentTorregrosa/drupal-scaffold that referenced this pull request Apr 21, 2018

@FlorentTorregrosa FlorentTorregrosa force-pushed the FlorentTorregrosa:master branch from 331c486 to d0d450f Apr 21, 2018

FlorentTorregrosa added a commit to FlorentTorregrosa/drupal-scaffold that referenced this pull request Apr 21, 2018

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2018

Hello,

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

Would it be possible to merge it please?

@FlorentTorregrosa FlorentTorregrosa force-pushed the FlorentTorregrosa:master branch from bbf9fa6 to 554467b Jul 5, 2018

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

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

This comment has been minimized.

Copy link

commented Aug 29, 2018

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"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.