Handle file copy error in drush basic-install on multisite [#2717331]

The drush installation script don't seem to work on multisite installations. The origin directory is always set to themes/basic and destination is set to 'themes/' . $machine_name. Not making it possible to place it within the current site's directory eg. sites/example.com/themes.

Comment	File	Size	Author
#10	interdiff.txt	1.45 KB	ooystein
#10	basic.theme-copy-error-on-multisite-2717331-10.patch	6.26 KB	ooystein
#7	basic.theme-copy-error-on-multisite-2717331-7.patch	6.34 KB	ooystein
#5	basic.theme-copy-error-on-multisite-2717331-5.patch	6.34 KB	ooystein
#2	basic.theme-copy-error-on-multisite-2717331-2.patch	983 bytes	ooystein

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

2 May 2016 at 13:02

ooystein created an issue. See original summary.

Comment #2

ooystein CreditAttribution: ooystein at Ramsalt Lab commented 2 May 2016 at 13:07

Status:

Active

» Needs review

File	Size
basic.theme-copy-error-on-multisite-2717331-2.patch	983 bytes

My attached patch does not fix the issue with multisites, but it fixes some issues with error handling and error messages.

The patch makes sure the script stoppes execution when it can't find themes/basic to copy to the temp directory.

This patch also fixes a problem with a placeholder for an existing error message that doesn't match the placeholder in the args array.

Comment #3

ooystein CreditAttribution: ooystein at Ramsalt Lab commented 13 May 2016 at 23:39

Status:

Needs review

» Needs work

I'm working on the full patch to make the script compatible with multisite setups and to allow the basic theme to be placed in other places than just "themes/basic".

Comment #4

leahtard CreditAttribution: leahtard at The Jibe commented 14 May 2016 at 15:33

Thanks ooystein! Happy to help test when it is ready.

Cheers, Leah

Comment #5

ooystein CreditAttribution: ooystein at Ramsalt Lab commented 14 May 2016 at 22:14

Status:

Needs work

» Needs review

File	Size
basic.theme-copy-error-on-multisite-2717331-5.patch	6.34 KB

This patch provides support for placing basic where ever you like and running the script on multisite setups.

I added a drush option to enable users specifying the destination for the new clone of the theme (This was maybe possible before, but in newer Drush versions it seems the option don't work unless it's specified in the hook_drush_command array)

When no option is supplied by the user the script will figure out the correct destination based on the environment (DRUSH_DRUPAL_SITE_ROOT).

But I noticed that in function drush_basic_install() after the return; on line 126, there is a lot of dead code. Is there any reason it's kept there? Should probably remove it if not.

Comment #6

joelpittet

English

Vancouver

CreditAttribution: joelpittet commented 17 May 2016 at 19:36

This looks great, maybe @leahtard can manually test it but I think from the looks of it we can commit this right away. There is a couple things to clean up on commit but if you'd like you can touch them up in a new patch for us @ooystein?

+++ b/includes/basic.drush.inc
@@ -77,24 +80,30 @@ function drush_basic_install() {
+  if(!basic_copy_dir($origin_path, $temp_path)) {

@@ -109,7 +118,12 @@ function drush_basic_install() {
+  if(!basic_copy_dir($temp_path, $destination_path)) {

nit: space after if.

+++ b/includes/basic.drush.inc
@@ -109,7 +118,12 @@ function drush_basic_install() {
+      '!temp_path' => $temp_path,
+      '!destination_path' => $destination_path,

They handle these legacy arguments but could we replace them? https://github.com/drush-ops/drush/blob/659f67712adab143a2c921f9f2382878...

+++ b/includes/basic.drush.inc
@@ -273,7 +313,7 @@ function drush_basic_require_valid_theme_name($message, $default = NULL) {
-    return drush_set_error('INVALID_PATH', dt('The given path @path is not a directory.', array(
+    return drush_set_error('INVALID_PATH', dt('The given path !path is not a directory.', array(
       '!path' => $path,

We actually want to use the @ here, even though thanks for correcting it! The ! was removed in drupal 8.

Thanks for also mentioning the dead code. Likely we should remove it but maybe not here.

Comment #7

ooystein CreditAttribution: ooystein at Ramsalt Lab commented 18 May 2016 at 18:04

File	Size
basic.theme-copy-error-on-multisite-2717331-7.patch	6.34 KB

Thanks for looking it over.

1. Good catch. Fixed in attached patch.

2. & 3. I have tried to find out what Drush decided on regarding this, but I haven't managed to find a clear answer on this. Drush GitHub Issue #1637 and issue #1647 ends up creating the find_legacy_dt_args() function and not saying if we should stop using "!" for placeholders. There are use cases in Drush that makes it reasonable to keep "!", so maybe they will keep it.

Anyway, if the theme maintainers want to switch to "@" I have no problem updating the patch. Just give me an update and I'll fix it!

Comment #8

leahtard CreditAttribution: leahtard at The Jibe commented 31 May 2016 at 00:28

Thanks ooystein. It looks like we are close. However, I have one issue. When I attempt to apply this patch I get:

$ git apply --check basic.theme-copy-error-on-multisite-2717331-7.patch
fatal: unrecognized input

I found this post: http://stackoverflow.com/questions/13675782/git-shell-in-windows-patchs-.... Is it relevant?

I was wondering if someone else can please try the patch let me know if they experience the same issue?

Cheers, Leah

Comment #9

joelpittet

English

Vancouver

CreditAttribution: joelpittet commented 31 May 2016 at 07:58

Status:

Needs review

» Needs work

Looks like it needs a re-roll. The output I got:

patching file includes/basic.drush.inc
Hunk #5 FAILED at 167.
Hunk #6 FAILED at 175.
Hunk #7 FAILED at 188.
Hunk #8 succeeded at 218 (offset -51 lines).
Hunk #9 succeeded at 262 (offset -51 lines).
3 out of 9 hunks FAILED -- saving rejects to file includes/basic.drush.inc.rej

Comment #10

ooystein CreditAttribution: ooystein at Ramsalt Lab commented 31 May 2016 at 09:05

File	Size
basic.theme-copy-error-on-multisite-2717331-10.patch	6.26 KB
interdiff.txt	1.45 KB

Here is the re-roll. I also did a small improvement to the patch so I added an interdiff.

Comment #11

31 May 2016 at 16:54

ooystein authored 6152581 on 8.x-1.x

Issue #2717331 by ooystein, leahtard, joelpittet: Handle file copy error...

Comment #12

leahtard CreditAttribution: leahtard at The Jibe commented 31 May 2016 at 16:56

Status:

Needs work

» Fixed

Thanks for the new patch. I was able to successfully apply this one. I ran the renaming script and all worked well. Thanks for your help with this ooystein!

Cheers, Leah