url() in hook_requirements breaks install profiles

Patch looks good.

Hi there - I was creating an installation profile for a configuration that includes jquery_ui.module, and I ran into the problem that required this patch. After I patched the module from the latest dev snapshot (Nov 4) and got the profile working, I see now that the javascript file paths are incorrect. I get for example (from HTML head):

<script type="text/javascript" src="/Applications/Bonbon-Till/root_drupal//Users/ZMan9854/Sites/Applications/Bonbon-Till/root_drupal/sites/all/modules/jquery_ui/jquery.ui/ui/minified/ui.progressbar.min.js?r"></script>

when it was previously:

<script type="text/javascript" src="/Applications/Bonbon-Till/root_drupal/sites/all/modules/jquery_ui/jquery.ui/ui/minified/ui.progressbar.min.js?r"></script>

Forgive me if it's just my misconfiguration : ) Thanks!

@Zman9854: Thanks for testing and reporting this. Looks like a reasonable effect of the proposed change to me.

Slightly modified version of the above patch. Works well for me.

The only change is doing a check on the MAINTENANCE_MODE constant before choosing whether to use dirname or drupal_get_path.

Hope it helps,

James

This version gets rid of a php notice. I think it's good to go.

(please ignore, wrong issue)

Thanks for reporting, reviewing, and testing! Committed to all branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

I am afraid this is still an active issue.

When jquery is not there, the link to the README also uses drupal_get_path() (and url()) which both functions executes db queries on tables that are not there.

This went undetected probably because the previous patches were tested with a module with jquery.ui installed.

I am attaching a patch that tries to solve this using something similar to the above solutions, although with a twist as now it has to be a link. The change is within the t() description as is the following:

url(drupal_get_path('module', 'jquery_ui') . '/README.txt')

for

dirname(str_replace(getcwd(), '', __FILE__)) . '/README.txt'

I am honestly not very proud about this patch, specially wondering if this will work on windows, but I couldn't find any other way around it. My real suggestion would be to remove that link altogether and just leave README.txt as plain text (no link).

I can confirm the patch in #10 works: we were still having problems here too. As the patch shows the relevant problem comes from the url() call to create the link to the README.txt in the "Requirements problems" warning message if you don't have jquery.ui installed:

Fatal error: Call to undefined function db_query() in /var/www/drupal/drupal-6/includes/path.inc on line 55

This patch solves that error for me.

Thanks for reporting, reviewing, and testing! Committed.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Hi,
Sorry to re-open this, but I thought it better to do it here, instead of starting a new issue, as the suggested solution in the patch in #10 does not work in all cases. It might be fine within an install profile, but when simply adding the module to an existing installed site, the readme link can be incorrectly formed.

The problem is that the derived path using dirname(str_replace(getcwd(), '', __FILE__)) contains a / at the start which means the href location is built from the host but excludes the path to the drupal root. This may not be apparent on some sites, but on mine the path to the drupal root
is http://localhost:8888/mysite whereas the code as it stands in the latest dev version, creates an href of http://localhost:8888/sites/all/files/modules/jquery_ui/README.txt, ie the path 'mysite' is missing.

If the leading slash is removed this does not help because the href location then becomes relative and we get
http://localhost:8888/mysite/admin/build/sites/all/modules/jquery_ui/REA...

So, when not in installation mode, I think the correct way to generate links to specific files is to start the link with $GLOBALS['base_url'] which will give http://localhost:8888/mysite. This is how links to file attachments are created in file_create_url() in /includes/file.inc. The problem with using the url() function, even when the db is available, is that it is designed for urls and adds ?q= if clean urls are not enabled, which does not work either, because the path is not found in the menu router table.

We could test for MAINTENANCE_MODE == 'install' just as is done in the patch in #6 and leave the result unchanged for during installation (if what is currently coded works, which I have not tested but the thread above implies is OK) but change it to use $GLOBALS['base_url'] so that it works correctly when the db is installed.

If you think that is the way to do it let me know and I'll make a patch. Or if I have got it all wrong, let me know that too.

Jonathan

Still experiencing this with 6.x-1.3 which was released on June 20--after the April 2 commit.

I'm trying to use the hostmaster installation profile:

#0  drupal_lookup_path(alias, /README.txt, ) called at [/Users/amorton/Sites/hostmaster/includes/path.inc:112]
#1  drupal_get_path_alias(/README.txt, ) called at [/Users/amorton/Sites/hostmaster/includes/common.inc:1489]
#2  url(/README.txt) called at [/Users/amorton/Sites/hostmaster/sites/all/modules/jquery_ui/jquery_ui.install:34]
#3  jquery_ui_requirements(install)
#4  call_user_func_array(jquery_ui_requirements, Array ([2] => install)) called at [/Users/amorton/Sites/hostmaster/includes/module.inc:462]
#5  module_invoke(jquery_ui, requirements, install) called at [/Users/amorton/Sites/hostmaster/includes/install.inc:692]
#6  drupal_check_profile(hostmaster) called at [/Users/amorton/Sites/hostmaster/install.php:920]
#7  install_check_requirements(hostmaster, ) called at [/Users/amorton/Sites/hostmaster/install.php:115]
#8  install_main() called at [/Users/amorton/Sites/hostmaster/install.php:1180]

Fatal error: Call to undefined function db_result() in /Users/amorton/Sites/hostmaster/includes/path.inc on line 56

Okay I think we can get around this simply by passing the 'alias' => TRUE option url() to indicate that it shouldn't look for a path alias.

Modifying the subject to match the remaining issue.

@15 Drewish, I think you are mistaken with your versions. 1.3 was released on 21 June 2009 according to the project page and the .info (you may have saw June and thought this was 2010). It is only the dev version which contains the commit of 2nd April 2010, and the url() call has been removed because line 34 in .install is now:

    $requirements['jquery_ui']['description'] = $t('The <a href="@jqueryui">jQuery UI</a> plugin is missing. <a href="@download">Download</a> and extract it to your <em>jquery_ui</em> module directory. See <a href="@readme">README.txt</a> for more info.', array(
      '@jqueryui' => 'http://jqueryui.com',
      '@download' => 'http://code.google.com/p/jquery-ui/downloads/list?q=1.6',
      // drupal_get_path() breaks install profiles.
      '@readme' => dirname(str_replace(getcwd(), '', __FILE__)) . '/README.txt',
    ));

My question in #14 remains open, though.

Jonathan

Yup, I was mistaken. Pretend I asked for a new stable release ;)

erm, so the remaining issue is fixed already? Or what's left here?

I think it's fixed, #18 is right just looked at the code. Maybe it's time for a new stable release?

I've just checked the latest dev, and hook_requirements() in jqueryui.install has not changed since the problem I reported on 19th June. So the error in #14 is not resolved. What do you think of my suggestion I made then:

We could test for MAINTENANCE_MODE == 'install' just as is done in the patch in #6 and leave the result unchanged for during installation (if what is currently coded works, which I have not tested but the thread above implies is OK) but change it to use $GLOBALS['base_url'] so that it works correctly when the db is installed.

I am happy to code this and make a patch if you think it is the correct way to do it.

It's not my call to make, but I think that if your suggestion of using $_GLOBAL['base_ref'] or rather base_path() which is the drupal equivalent to that works for both installation profiles/normal installation of the module that should be what you should provide a patch for rather than testing for something which is not really need it.

@23, Hanoii, yes of course you are right that it is better to find one fix which works in both scenarios. I don't know much about installation profiles, I have tried to read the drupal documentation but I've not found out yet whether $GLOBALS['base_url'] is defined at that stage. Could anyone tell me?

jonathan1055, it should be. $GLOBALS['base_url'] is defined in bootstrap.inc very early, specifically in conf_init(). The install system is mostly a normal Drupal bootstrap, just like any other page load.

I'm tempted to remove that link to README.txt entirely. This just seems to be a major waste of developer time, which could be better spent with far more important improvements.

@sun, second that :)

@25 Thanks James for the info. The solution is really simple now! Just need to add $GLOBALS['base_url'] infront of the dirname(str_replace(getcwd(), '', __FILE__)) to make it an absolute ref containing the correct full path. This should work for all cases, so we do not have to test for installation mode.

Attached is a patch against the current dev release (11th July)

No, that doesn't work for Drupal installed in a sub-directory...

I've tested it on a drupal installation in a sub-directory on my localhost and it seems to work fine. The attached screenshot shows the module page in a drupal site installed at drupal6_in_subdir/main. Also added my standard block of global variables at the right hand side. $GLOBALS['base_url'] is http://localhost:8888/drupal_6_in_subdir/main. The hover text in the status bar shows the correct link.

I must be mis-understanding what you mean, as it seems OK to me.

Jonathan

+1 to just removing the url() to the README file, though I'll probably test the patch in #28 too.

Thanks for the confirmation. Done, in all branches.

The patch does not work in #28 when the site is in a subdirectory for me. See the attached screenshot. It does work if it's the root site (example.com).

Sorry, crossposted.