Skip to content
This repository has been archived by the owner on Feb 13, 2023. It is now read-only.

Make Drupal VM into a Composer plugin that scaffolds a Vagrantfile #1256

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

oxyc
Copy link
Collaborator

@oxyc oxyc commented Mar 24, 2017

Proof of concept for #1247 which supports all earlier setups with the addition of reading config_dir from composer.json.

Might want to drop support for some scenarios at some point tbh...

@geerlingguy
Copy link
Owner

Like basically say "Add Drupal VM as a composer dependency, or use it standalone, and that's it"?

@oxyc
Copy link
Collaborator Author

oxyc commented Mar 24, 2017

Yeah, that would probably make it easier.

Just realized that making this change would break all existing composer dependency setups as the old Vagrantfile will be overridden and the DRUPALVM_CONFIG_DIR definition will disappear.

@geerlingguy
Copy link
Owner

break all existing composer dependency setups

😩

@geerlingguy
Copy link
Owner

Also, with the Vagrantfile ever-expanding in size, I wonder if it might be good to have a 'ruby-src subfolder somewhere (something like that... whatever the Ruby convention would be) and break out business logic in there?

Or if there's some other way of building up a complicated Vagrantfile that's a better pattern. It's just getting unwieldy at this point.

@oxyc
Copy link
Collaborator Author

oxyc commented Mar 24, 2017

@oxyc
Copy link
Collaborator Author

oxyc commented Mar 25, 2017

This is what it would look like if we dropped support for a delegating Vagrantfile.

Edit: Added a commit so it's easier to read. I can always revert it.

If we decide to go with this I guess the PR will have to wait until Drupal VM 5.0.0 as this will truly break all the things.

if Dir.exist?("#{host_drupalvm_dir}/vendor/geerlingguy/drupal-vm")
host_drupalvm_dir = "#{host_project_dir}/vendor/geerlingguy/drupal-vm"
guest_drupalvm_dir = "#{guest_project_dir}/vendor/geerlingguy/drupal-vm"
end
Copy link
Collaborator Author

@oxyc oxyc Mar 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One annoying thing here is that this wouldn't support forks... any ideas?

@oxyc oxyc force-pushed the composer-plugin branch 2 times, most recently from 8474fae to e37be08 Compare March 25, 2017 01:45
@oxyc oxyc changed the title [WIP] Add Composer plugin which scaffolds a Vagtranfile [WIP] Add Composer plugin which scaffolds a Vagrantfile Mar 25, 2017
@thom8
Copy link
Collaborator

thom8 commented Mar 26, 2017

break all existing composer dependency setups

This is actually the reason we are dropping all other installation methods so it's easier/safer to make BC breaks - beetboxvm/beetbox#391 (comment)

But yeah these changes are only coming in the next minor version bump.

@thom8
Copy link
Collaborator

thom8 commented Mar 26, 2017

@oxyc there's also a test Class which could be useful -- https://github.com/beetboxvm/beetbox/blob/master/tests/PluginTest.php

@oxyc
Copy link
Collaborator Author

oxyc commented Apr 11, 2017

  • approval
  • update docs
  • should we include tests for the composer plugin?
  • way more readable logic

@oxyc
Copy link
Collaborator Author

oxyc commented Apr 23, 2017

Also, with the Vagrantfile ever-expanding in size, I wonder if it might be good to have a 'ruby-src subfolder somewhere (something like that... whatever the Ruby convention would be) and break out business logic in there?

#1332

@geerlingguy
Copy link
Owner

@oxyc - Also, note that I think I'd like to include #1332 in a 4.x release, but I'm considering saving this change (along with more of a "this is how you do it the Drupal VM way" philosophy) for a 5.0 release. What do you think?

@oxyc
Copy link
Collaborator Author

oxyc commented Apr 23, 2017

Yeah I agree this should be saved for 5.0.

Btw, is there a way to detect which version you're updating from in a composer plugin? If it is, we could issue a warning stating that the Vagrantfile has been overridden and that config_dir now needs to be set in composer.json.

@geerlingguy
Copy link
Owner

Is there a way to detect which version you're updating from in a composer plugin?

That... I'm not sure of. I'm pretty new to Composer plugins myself.

@oxyc
Copy link
Collaborator Author

oxyc commented Apr 24, 2017

I came pretty far and managed to detect updates from <5.0.0 using Composer\Installer\PackageEvents::POST_PACKAGE_UPDATE but then I noticed that it won't run when the package is first updated, it will run the old code until the next update.

So I went a different, hackier, way. Thoughts?

screen shot 2017-04-23 at 19 10 32

@oxyc oxyc force-pushed the composer-plugin branch 3 times, most recently from c88d57e to df17e38 Compare April 24, 2017 16:04
@oxyc
Copy link
Collaborator Author

oxyc commented Apr 24, 2017

Also, note that I think I'd like to include #1332 in a 4.x release, but I'm considering saving this change (along with more of a "this is how you do it the Drupal VM way" philosophy) for a 5.0 release. What do you think?

Or we could just aim to release 5.0.0 at or shortly after DrupalCon? Like we did with 3.0.0 last year. Maybe too early for another major version bump though...

@oxyc
Copy link
Collaborator Author

oxyc commented Apr 24, 2017

Regarding docs for this I guess we could entirely delete Drupal VM as a Composer Dependency, or move it as a brief page under_Getting Started_ section somewhere.

In the Installation docs we point people to the Quick Start Guide. I guess we should either update the Quick Start Guide, or update these sections with how to set up a project using Drupal VM as a Composer dependency.

Basically the install instructions are

  1. Require Drupal VM as a Composer development dependency.

    composer require --dev geerlingguy/drupal-vm
    
  2. Create and configure a config.yml file, eg. in vm/config.yml.

  3. If you placed the config.yml file in a subdirectory tell Drupal VM about it. If not, Drupal VM will look for it in the root of your project.

     composer config extra.drupalvm.config_dir 'vm'
    

@geerlingguy
Copy link
Owner

@oxyc - I think one other thing that would be helpful to document somewhere/somehow is how you could still patch Drupal VM using something like https://github.com/cweagans/composer-patches — sometimes people need to either use a change in master but not a tagged release, or patch one specific task somewhere for their project...

@oxyc
Copy link
Collaborator Author

oxyc commented Apr 24, 2017

Maybe add as the first page Installation / How to integrate Drupal VM (I'm terrible at naming things)

Where we point out that there are two ways of integrating Drupal VM with your project.

  1. You download Drupal VM and build a quick site within Drupal VM using vagrant up. Point out the downsides.

  2. You require Drupal VM as a development dependency to your existing project. Show a quick start using drupal-composer/drupal-project

Then we also point to this page from the separate Install pages so people consider the options.

@geerlingguy
Copy link
Owner

@oxyc - I'm good with that. As long as you don't make people who use it standalone feel shameful, since that's probably going to still be the default/majority use case for a while until more of the community adopts Composer.

@oxyc
Copy link
Collaborator Author

oxyc commented Apr 24, 2017

that's probably going to still be the default/majority use case for a while

Yeah this would probably still be the first option mentioned in docs. What about the Quick Start Guide though? Should that be updated to reflect both or should we keep composer dependency as an advanced feature?

patch Drupal VM using something like https://github.com/cweagans/composer-patches

Mind blown! I hadn't even considered that.

@@ -1,5 +1,7 @@
Drupal VM is configured to use `composer create-project` to build a Drupal 8 codebase by default but supports building Drupal from a custom `composer.json` file as well.

_Note that if you already have a project built using a `composer.json` file you should instead add [Drupal VM as a dependency](composer-dependency.md) of your project. The method described below will make it somewhat difficult to manage your `composer.json` together with Drupal VM as the file will be copied from `drupal_composer_path` into `drupal_composer_install_dir` as a secondary `composer.json`._
Copy link
Collaborator Author

@oxyc oxyc Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you okay with this, or maybe too scary? Honestly I think we should default drupal_composer_path to no in favor of Drupal VM as a dependency. I think we've seen a few issues due to this particular build setup being a bit non intuitive and I dont see much use case for it other than a faster version of drupal_build_composer_project.

Copy link
Collaborator Author

@oxyc oxyc Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or well that's a bit odd too. Maybe it should be renamed drupal_composer_copy_path so it makes more sense as to why you need to set it to false for the composer dependency setup.

cconfig = load_composer_config("#{host_project_dir}/composer.json")

# If Drupal VM is a Composer dependency set the correct path.
drupalvm_path = "#{vendor_dir}/geerlingguy/drupal-vm"
Copy link
Collaborator Author

@oxyc oxyc Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vendor_dir being a function seems very unclear to me coming from PHP. Rubocop complains if I add unnecessary parenthesis though. Maybe I should drop that lint?

Vagrantfile Outdated
@@ -3,17 +3,28 @@

require './lib/drupalvm/vagrant'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derp this fails now.


# If Drupal VM is a Composer dependency set the correct path.
vendor_dir = composer_conf.fetch('extra', {}).fetch('vendor-dir', 'vendor')
drupalvm_path = "#{vendor_dir}/geerlingguy/drupal-vm"
Copy link
Collaborator Author

@oxyc oxyc Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem here. There are some Windows developers who might prefer to code entirely within the VM right? This is why I'm avoiding calls to composer without the which function which is now in the lib we dont have access to before figuring out host_drupalvm_path (d'oh). As I'm avoiding calling composer config to retrieve the vendor-dir path, this could theoretically be wrong if the user somehow set a custom vendor path using a different method than the vendor-dir extra-field.

Also this setup requires the user to run composer install to download Drupal VM. That's probably alright, I mean all window devs using this setup would have composer installed, right?

Copy link
Collaborator Author

@oxyc oxyc Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also allow the user to specify the path to Drupal VM in the composer.json. So that it doesnt have to reside within the vendor-dir. But I doubt anyone would need to use that.

drupalvm_path = cconfig['path'] || "#{vendor_dir}/geerlingguy/drupal-vm"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this better than depending on composer being available in the current session.

Also need to consider env vars -- https://getcomposer.org/doc/03-cli.md#composer-vendor-dir

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some Windows developers who might prefer to code entirely within the VM right?

Yes. (And the occasional Mac or Linux user who likes the isolation.)

Also this setup requires the user to run composer install to download Drupal VM. That's probably alright, I mean all window devs using this setup would have composer installed, right?

Preferably, yes. But right now there are a lot of Windows devs who neither want nor are allowed to set up PHP and/or Composer on Windows... but it could be possible we could make that a requirement for 5.0.0+...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But right now there are a lot of Windows devs who neither want nor are allowed to set up PHP and/or Composer on Windows.

Would these devs still be using Drupal VM as a composer dependency?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also the possibility that someone commits all dependencies and therefore not all Devs would need composer..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for example a tech lead might use composer on behalf of the rest of the team and track all dependencies - https://www.codeenigma.com/build/blog/do-you-really-need-composer-production

I personally prefer an isolated build process but some might use DVM as composer dep without composer

If you're not using `composer` in your project you can still download Drupal VM (or add it as a git submodule) to any subdirectory in your project. As an example let's name that directory `box/`.
# Set the location of the composer.json file and Drupal core.
drupal_composer_install_dir: "/var/www/drupalvm"
drupal_core_path: "{{ drupal_composer_install_dir }}/web"
Copy link
Collaborator Author

@oxyc oxyc Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be too much to print this in the console if Drupal VM is added and no prior Vagrantfile exists? It would just make getting started so much easier if you wouldn't have to visit the docs... Or scaffold the vm/config.yml file in case a Vagrantfile is missing.

@oxyc oxyc mentioned this pull request Jul 8, 2017
@geerlingguy
Copy link
Owner

Just wanted to note that I grabbed some of the work here and dumped it into https://github.com/geerlingguy/drupal-vm-docker, which is more of a quick proof-of-concept just using the Drupal VM Docker container as a starting point.

I am still planning on eventually converting Drupal VM itself into a Composer Plugin—maybe for 5.0.0 still—but I don't think the timing is right yet; it seems the majority of people using Drupal VM still download or git clone it, and aren't ready to take the full Composer plunge.

But we can use https://github.com/geerlingguy/drupal-vm-docker as a testing ground for this, too.

@geerlingguy geerlingguy modified the milestones: 5.0.0, 6.0.0 Apr 4, 2019
@geerlingguy
Copy link
Owner

Dropping to 6.0.0 milestone. The Drupal community's adoption of Composer is just not there yet to default this thing to Composer-first :(

@geerlingguy geerlingguy modified the milestones: 6.0.0, 7.0.0 Jul 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants