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

Install Drupal modules/themes/profiles to the correct location #65

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 126 additions & 4 deletions src/Composer/Installers/DrupalInstaller.php
Original file line number Diff line number Diff line change
@@ -1,12 +1,134 @@
<?php
namespace Composer\Installers;

/**
* The Drupal Installer for Composer Installers.
*
* This locates Drupal's root directory, and installs into sites/all if
* the path is available. Note that much of this was adopted from Drush.
*
* @link http://drupal.org/project/drush
*/
class DrupalInstaller extends BaseInstaller
{
protected $locations = array(
'module' => 'modules/{$name}/',
'theme' => 'themes/{$name}/',
'profile' => 'profiles/{$name}/',
'drush' => 'drush/{$name}/',
'module' => '{$root}/modules/{$name}/',
'theme' => '{$root}/themes/{$name}/',
'profile' => '{$root}/profiles/{$name}/',
'drush' => '{$root}/drush/{$name}/',
);

/**
* {@inheritDoc}
*/
public function inflectPackageVars($vars)
{
// Find Drupal's root directory.
$root = $this->locateDrupalRoot();
if ($root) {
// If we are installing on Drupal 5, 6 or 7, use "sites/all".
if (version_compare($this->drupalMajorVersion($root), '8', '<')) {
$root .= '/sites/all';
}
}
else {
// If it's not a valid directory, then use the current directory.
$root = '.';
}

// Inject Drupal's root directory into the variables array.
$vars['root'] = $root;

return $vars;
}

/**
* Checks whether given path qualifies as a Drupal root.
*
* @param $path
* The relative path to common.inc (varies by Drupal version), or FALSE if
* not a Drupal root.
*/
protected function validDrupalRoot($path) {
if (!empty($path) && is_dir($path)) {
$candidates = array('includes/common.inc', 'core/includes/common.inc');
foreach ($candidates as $candidate) {
if (file_exists($path . '/' . $candidate)) {
return $candidate;
}
}
}

return false;
}

/**
* Exhaustive depth-first search to try and locate the Drupal root directory.
*/
protected function locateDrupalRoot($path = '')

Choose a reason for hiding this comment

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

I haven't done drupal development in quite some time; so i'm not familiar with current development and composer practices with it, but i would almost expect drupal to be in a child directory just as often as a parent directory. is that case covered with this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't scan child directories, nor am I sure if that should be a considered use case. If you're doing Drupal stuff like that, then you have symlinks set up to handle such a thing. Most cases though, it would be a parent directory.

Worth considering further down the line though! What are your thoughts?

Choose a reason for hiding this comment

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

I know drupal has changed significantly since i've worked with it... I was accustomed to working with it fairly similar to how i interact with wordpress. Now that it has a symfony stack it's significantly different, with wordpress it needs to be within the webroot and my standard implementation has the root composer.json above the wordpress directory. However, in that case it may be getting into handling webroot packages which i believe is outside of the desired scope of this project. I created a separate installer for that use case, and i have PR #52 to take care of the ability to manipulate where the project lives in the root composer.json. Maybe implementation of child directory installations would not be necessary with that PR being merged in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very nice PR, @slbmeh. I'll be sure to read through it closer soon. Your work on getting Wordpress to be composer.json-compliant is great, and definitely something that I see the DrupalInstaller benefiting from.

This PR mimics the effect that the Drupal command line utility has, and wouldn't require the extra notes in most composer.json files. I understand having a composer.json in a child path and not within the root is somewhat backwards from a best practice Composer project, but that is unfortunately how Drupal is designed. There are plans to change that within the next couple years though, however, for Drupal 9. We could adapt this solution quite easily to match accordingly by then.

{
$path = empty($path) ? getcwd() : $path;
do {
if ($this->validDrupalRoot($path)) {
return $path;
}
}
while ($path = dirname($path) && $path != DIRECTORY_SEPARATOR);

return false;
}

/**
* Return the user's home directory.
*/
protected function serverHome() {
$home = getenv('HOME');

if (empty($home)) {
if (!empty($_SERVER['HOMEDRIVE']) && !empty($_SERVER['HOMEPATH'])) {
// On a Windows server.
$home = $_SERVER['HOMEDRIVE'] . $_SERVER['HOMEPATH'];
}
}

return empty($home) ? NULL : $home;
}

/**
* Retrieves Drupal version.
*
* @return string A version string similar to 7.22-dev.
*/
protected function drupalVersion($drupal_root = null) {
if (($drupal_root != null) || ($drupal_root = $this->locateDrupalRoot())) {
// Drupal 7 stores VERSION in includes/bootstrap.inc, while Drupal 8
// uses core/includes/bootstrap.inc.
$version_constant_paths = array('/modules/system/system.module', '/includes/bootstrap.inc', '/core/includes/bootstrap.inc');
foreach ($version_constant_paths as $path) {
if (file_exists($drupal_root . $path)) {
require_once $drupal_root . $path;
}
}

if (defined('VERSION')) {
return VERSION;
}
}
}

/**
* Retrieves Drupal's major version number.
*/
protected function drupalMajorVersion($drupal_root = null)
{
$major_version = false;
if ($version = $this->drupalVersion($drupal_root)) {
$version_parts = explode('.', $version);
if (is_numeric($version_parts[0])) {
$major_version = (integer) $version_parts[0];
}
}

return $major_version;
}
}
8 changes: 4 additions & 4 deletions tests/Composer/Installers/Test/InstallerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,10 @@ public function dataForTestInstallPath()
array('cakephp-plugin', 'Plugin/Ftp/', 'shama/ftp'),
array('codeigniter-library', 'libraries/my_package/', 'shama/my_package'),
array('codeigniter-module', 'modules/my_package/', 'shama/my_package'),
array('drupal-module', 'modules/my_module/', 'shama/my_module'),
array('drupal-theme', 'themes/my_module/', 'shama/my_module'),
array('drupal-profile', 'profiles/my_module/', 'shama/my_module'),
array('drupal-drush', 'drush/my_module/', 'shama/my_module'),
array('drupal-module', './modules/my_module/', 'shama/my_module'),

Choose a reason for hiding this comment

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

These test cases only cover the current working directory and don't cover the added use case of the root being in a parent directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure how to test when installing the module into a sites/all/modules directory. It checks for a bootstrap.inc file with a defined VERSION constant. Not sure how to have the tests cover such a case. If you have any ideas, then I'd be more than open to any thoughts you have!

Thanks.

Choose a reason for hiding this comment

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

I think these changes cover the move from the previous functionality to the current, but I imagine you would need to implement 2 new test cases to cover parent directory and current directory sites/all/modules directories. It's what I had to do with my package type pr changes. Sorry to keep mentioning it, but I think you may be trying to achieve with drupal what I wanted to achieve with wordpress. Test Case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we could split up the functions into separate tests, test them all individually, and possibly use a Resources directory for tests on file access-based installs. I'll see if I could come up with some tests around these. Thanks @slbmeh!

We are trying to accomplishing similar things with both PRs, and I see the DrupalInstaller benefiting from having both. Despite peoples' arguments, Drupal and Wordpress have quite similar architectures and I love when both the Wordpress and Drupal communities collide 👍 .

array('drupal-theme', './themes/my_module/', 'shama/my_module'),
array('drupal-profile', './profiles/my_module/', 'shama/my_module'),
array('drupal-drush', './drush/my_module/', 'shama/my_module'),
array('fuel-module', 'fuel/app/modules/module/', 'fuel/module'),
array('fuel-package', 'fuel/packages/orm/', 'fuel/orm'),
array('joomla-plugin', 'plugins/my_plugin/', 'shama/my_plugin'),
Expand Down