The devel Drush command help states -

$ drush help devel-download
Downloads the FirePHP library from http://firephp.org/.

Arguments:
 path                                      Optional. A path to the download folder. If omitted 
                                           Drush will use the default location                 
                                           (sites/all/libraries/firephp).

However, looking at the code, the opposite appears to be true -

  $args = func_get_args();
  if (isset($args[0])) {
    $path = $args[0];
  }
  else {
    $path = drush_get_context('DRUSH_DRUPAL_ROOT');
    if (module_exists('libraries')) {
      $path .= '/' . libraries_get_path('FirePHPCore') . '/FirePHPCore';
    }
    else {
      $path .= '/' . drupal_get_path('module', 'devel') . '/FirePHPCore';
    }
  }

The default location is actually inside of the Devel module itself. Only if the Libraries module is installed will FirePHP be located there.

I'm happy to provide a patch, but I'm not sure if the documentation or the code is wrong. Which was the intention here?

CommentFileSizeAuthor
#15 firephp_default-1456270-15.patch3.34 KBlussoluca
#13 firephp_default-1456270-13.patch4.41 KBlussoluca
#11 1456270-11-D8.patch797 bytesareke
#4 devel-wrong_firephp_default_location_docs-1456270-3-D6.patch766 byteserikwebb
#4 devel-wrong_firephp_default_location_docs-1456270-3-D7.patch800 byteserikwebb
#4 devel-wrong_firephp_default_location_docs-1456270-3-D8.patch773 byteserikwebb
#2 devel-wrong_firephp_default_location_docs-1456270-1.patch788 byteserikwebb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented

The patch should contain

    if (module_exists('libraries')) {
      $path .= '/sites/all/libraries/FirePHPCore';
    }

as the FirePHPCore is not downloaded yet. The statement $path .= '/' . libraries_get_path('FirePHPCore') . '/FirePHPCore'; is wrong when not having installed FirePHPCore.

erikwebb’s picture

erikwebb CreditAttribution: erikwebb commented
Status: Active » Needs review
FileSize
devel-wrong_firephp_default_location_docs-1456270-1.patch788 bytes

@clemens.tolboom - It looks like this portion has been fixed in #1179094: Drush integration devel-download wrong path download with libraries > 1.x.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Is this patch correct for D6 and D8 as well?

erikwebb’s picture

erikwebb CreditAttribution: erikwebb commented
FileSize
devel-wrong_firephp_default_location_docs-1456270-3-D8.patch773 bytes
devel-wrong_firephp_default_location_docs-1456270-3-D7.patch800 bytes
devel-wrong_firephp_default_location_docs-1456270-3-D6.patch766 bytes

Attached patches for each version. I tested D6 and D7, but not D8. The only dependent API is drupal_get_path(), so it should work the same.

Status: Needs review » Needs work

The last submitted patch, devel-wrong_firephp_default_location_docs-1456270-3-D8.patch, failed testing.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs work » Needs review

D7 passed, let's check D8.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

#4: devel-wrong_firephp_default_location_docs-1456270-3-D8.patch queued for re-testing.

erikwebb’s picture

erikwebb CreditAttribution: erikwebb commented
Version: 8.x-1.x-dev » 6.x-1.x-dev

D6?

erikwebb’s picture

erikwebb CreditAttribution: erikwebb commented

#4: devel-wrong_firephp_default_location_docs-1456270-3-D6.patch queued for re-testing.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Version: 6.x-1.x-dev » 8.x-1.x-dev

ALL GREEN, great. Now we need reviews.

areke’s picture

areke CreditAttribution: areke commented
Issue summary: View changes
FileSize
1456270-11-D8.patch797 bytes

The 8x patch didn't apply, so I rerolled it.

mimes’s picture

mimes CreditAttribution: mimes commented

Patches in #4 and #11 function as expected; the only issue I have is with the latest patch's naming scheme.

This issue has apparently existed for some time now, there was an issue in March and another one year ago.

lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
CreditAttribution: lussoluca commented
FileSize
firephp_default-1456270-13.patch4.41 KB

FirePHP and ChromePHP should be downloaded by Composer nowadays, so the devel-download command isn't so useful and we can remove it.

willzyx’s picture

willzyx CreditAttribution: willzyx commented

FirePHP and ChromePHP should be downloaded by Composer nowadays, so the devel-download command isn't so useful and we can remove it.

+1. For me is fine remove devel-download in the 8.x brach in favour of composer and move this issue to 7.x queue

+++ b/src/Plugin/Devel/Dumper/ChromePhp.php
@@ -39,7 +39,7 @@ class ChromePhp extends DevelDumperBase {
   public static function checkRequirements() {
-    return class_exists('ChromePhp', FALSE);
+    return class_exists('ChromePhp', TRUE);

+++ b/src/Plugin/Devel/Dumper/FirePhp.php
@@ -24,7 +24,8 @@ class FirePhp extends DevelDumperBase {
   public function dump($input, $name = NULL) {
-    fb($input);
+    $fb = new \FB();
+    $fb->dump($name, $input);

@@ -39,7 +40,7 @@ class FirePhp extends DevelDumperBase {
   public static function checkRequirements() {
-    return class_exists('FirePHP', FALSE);
+    return class_exists('FirePHP', TRUE);

This changes are unrelated. Please open another issue for that.. I will RTBC it instantly :)

lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
CreditAttribution: lussoluca commented
FileSize
firephp_default-1456270-15.patch3.34 KB

New patch without changes mentioned in #14

lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
CreditAttribution: lussoluca commented

This changes are unrelated. Please open another issue for that.. I will RTBC it instantly :)

Et voilà: #2735353: Find ChromePHP and FirePHP with Composer Autoloader

  • lussoluca committed d3d4932 on 8.x-1.x 
    Issue #1456270 by erikwebb, lussoluca, areke: FirePHP default location...
lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
CreditAttribution: lussoluca commented
Status: Needs review » Closed (fixed)

Committed and pushed to 8.x

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Closed (fixed) » Needs review

Moving to 7.x branch

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Version: 8.x-1.x-dev » 7.x-1.x-dev