Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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?
Comment | File | Size | Author |
---|---|---|---|
#15 | firephp_default-1456270-15.patch | 3.34 KB | lussoluca |
| |||
#13 | firephp_default-1456270-13.patch | 4.41 KB | lussoluca |
| |||
#11 | 1456270-11-D8.patch | 797 bytes | areke |
#4 | devel-wrong_firephp_default_location_docs-1456270-3-D6.patch | 766 bytes | erikwebb |
#4 | devel-wrong_firephp_default_location_docs-1456270-3-D7.patch | 800 bytes | erikwebb |
Comments
Comment #1
clemens.tolboomThe patch should contain
as the FirePHPCore is not downloaded yet. The statement
$path .= '/' . libraries_get_path('FirePHPCore') . '/FirePHPCore';
is wrong when not having installed FirePHPCore.Comment #2
erikwebb CreditAttribution: erikwebb commented@clemens.tolboom - It looks like this portion has been fixed in #1179094: Drush integration devel-download wrong path download with libraries > 1.x.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedIs this patch correct for D6 and D8 as well?
Comment #4
erikwebb CreditAttribution: erikwebb commentedAttached 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.
Comment #6
salvisD7 passed, let's check D8.
Comment #7
salvis#4: devel-wrong_firephp_default_location_docs-1456270-3-D8.patch queued for re-testing.
Comment #8
erikwebb CreditAttribution: erikwebb commentedD6?
Comment #9
erikwebb CreditAttribution: erikwebb commented#4: devel-wrong_firephp_default_location_docs-1456270-3-D6.patch queued for re-testing.
Comment #10
salvisALL GREEN, great. Now we need reviews.
Comment #11
areke CreditAttribution: areke commentedThe 8x patch didn't apply, so I rerolled it.
Comment #12
mimes CreditAttribution: mimes commentedPatches 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.
Comment #13
lussolucaFirePHP and ChromePHP should be downloaded by Composer nowadays, so the devel-download command isn't so useful and we can remove it.
Comment #14
willzyx CreditAttribution: willzyx commented+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
This changes are unrelated. Please open another issue for that.. I will RTBC it instantly :)
Comment #15
lussolucaNew patch without changes mentioned in #14
Comment #16
lussolucaEt voilà: #2735353: Find ChromePHP and FirePHP with Composer Autoloader
Comment #18
lussolucaCommitted and pushed to 8.x
Comment #19
willzyx CreditAttribution: willzyx commentedMoving to 7.x branch
Comment #20
willzyx CreditAttribution: willzyx commented