This patch adds a very small utility function, drupal_add_basepath, to common.inc. I am breaking this patch out of my more ambitious dialogs patch (http://drupal.org/node/193311) since that probably won't make it into D6. It is just silly that there are so many contrib module out there right now that all have the same line of code: 

drupal_add_js( array( 'mymodule' => array( 'base_path' => base_path() )), 'setting' );

Most modules that use Ajax need this line, and when I look at the source of a page, I will often see the base_path getting passed in two or more times.

In system.js you only get away with this line because you know Druapl.cleanURLsInstallCheck is only going to be bound on a single page with a known path. 

var url = location.protocol +"//"+ location.host + location.pathname.replace(/\/[^\/]*$/, "/") + "admin/settings/clean-urls/check";

could become 

var url = location.protocol +"//"+ location.host + Drupal.settings.base_path + "admin/settings/clean-urls/check";
CommentFileSizeAuthor
#25 base_path.patch2.56 KBWim Leers
#22 base_path.patch2.56 KBWim Leers
#18 base_path.patch2.17 KBWim Leers
#17 drupal_add_js_base_path_6.patch2.02 KBRobLoach
#14 add_basepath_5.patch2.72 KBRobLoach
#13 add_basepath_4.patch2.78 KBstarbow
#8 add_basepath.patch2.78 KBstarbow
#6 add_basepath_1.patch2.77 KBstarbow
#4 add_basepath.patch2.74 KBstarbow
#2 add_basepath.patch2.7 KBstarbow
add_basepath.patch753 bytesstarbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

+1 for idea, -1 on the execution though.

- Code is not conform the Drupal coding standards.
- "bathpath" instead of "base_path".
- You should implement the enhancement to Drupal.cleanURLsInstallCheck you're suggesting.

starbow’s picture

starbow CreditAttribution: starbow commented
FileSize
add_basepath.patch2.7 KB

Ok, I cleaned up the tabs and weird spelling (the danger of coding at 4pm). I ran it through coder and it didn't have any comments. And I patched install.php and system.js to implement the cleanup to Drupal.cleanURLsIncallCheck that I proposed. Works like a charm.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented 
+/*
+ * Provide base_path for javascript.  Needed for many Ajax calls

- Doxygen comment blocks start with "/**", not "/*".
- I think most of core uses "Javascript" instead of "javascript'. Sentences should be separated with one instead of 2 spaces. Dot missing at the end.
- "Needed for many Ajax calls" might be worded a bit better? I haven't got any suggestions though.

starbow’s picture

starbow CreditAttribution: starbow commented
FileSize
add_basepath.patch2.74 KB

Thanks.
I changed it to:

 /**
+ * Provide base_path for JavaScript.  
+ * base_path is needed in jQuery $.ajax calls, to build the url back to Drupal paths.
+ */
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

- "URL", not "url"
- You may need it in jQuery .load() calls too, and I'm sure others exist as well. Perhaps something in the lines of "The base path of a site is needed in dynamic requests to the server (AJAX/AHAH), to build the URL to Drupal paths."?

Sorry for all the nitpicking, but it wouldn't even be considered for core inclusion if all these details are right :)

starbow’s picture

starbow CreditAttribution: starbow commented
FileSize
add_basepath_1.patch2.77 KB

No apology needed. I really appreciate you taking the time to let me know what I need to do.

starbow’s picture

starbow CreditAttribution: starbow commented
starbow’s picture

starbow CreditAttribution: starbow commented
FileSize
add_basepath.patch2.78 KB

Changed the name of the function to drupal_add_base_path_to_js(), to make it clearer what it does.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

- Comments should wrap at col 78.
- Only one return after a function.

Then it seems ready to me.

profix898’s picture

profix898 CreditAttribution: profix898 commented

Just a thought: Why dont we add this automatically on every page that uses javascript (at least one call to drupal_add_js)? I mean many pages will use this to add a base path settings for custom js, so we could simply add this by default as we do with jquery.js and drupal.js ...

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

profix898: cool idea. That might work.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Needs work

I actually think that makes a lot of sense. It's used virtually everywhere, the overhead is negligible and it's much cleaner than adding another function.

Let's move the code there?

starbow’s picture

starbow CreditAttribution: starbow commented
Status: Needs work » Needs review
FileSize
add_basepath_4.patch2.78 KB

I don't know about adding the functionality to drupal_add_js. Obviously, base_path is very useful to have, but there is plenty of javascript to doesn't need it, and it seems pushy to require it for everyone.

This patch just has the two cleanup points Wim pointed out in comment #9.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community
FileSize
add_basepath_5.patch2.72 KB

I agree that sticking it in whenever you use Javascript is a bit much. Not all instances will use base_path.

The patch works well with the clean URLs check. Looks good to me! Here's the patch with the correct line endings.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

I disagree. The overhead of adding base_path into Drupal.settings is negligible, but the result is cleaner than what this patch is doing right now.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Reviewed & tested by the community » Needs work

Sounds good to me, let's add base_path in there.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Category: feature » task
Status: Needs work » Needs review
FileSize
drupal_add_js_base_path_6.patch2.02 KB

The attached patch adds Drupal.settings.base_path when it initializes Drupal.settings. It also uses Drupal.settings.base_path in the Clean URLs check as well as at installation. Mind testing?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Assigned: starbow » Wim Leers
Category: task » feature
FileSize
base_path.patch2.17 KB

I thought this patch was already committed a long time ago... Since it's not, finally a reroll!

It also fixes a missing semi-colon in system.js.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Add function to pass base_path to javascript » Automatically add Drupal.settings.base_path
yched’s picture

yched CreditAttribution: yched commented
Title: Automatically add Drupal.settings.base_path » Add function to pass base_path to javascript

Looks like #17 and #18 crossposted.

patch in #18 has :

+        'setting' => array(
+          array('base_path' => base_path()),
+        ),

while #17 has :

+        'setting' => array(
+          'base_path' => array('base_path' => base_path()),
+        ),

which one is correct ?

yched’s picture

yched CreditAttribution: yched commented
Title: Add function to pass base_path to javascript » Automatically add Drupal.settings.base_path

Hmm.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
FileSize
base_path.patch2.56 KB

After a rough grep(grep "location\.pathname" * -r), I couldn't find any other places in core that could benefit from this. So, here's the only missing other occurrence where Drupal.settings.basepath can be used (the one that Rob didn't miss and I did).

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Change looks sensible and fixes bug like http://drupal.org/node/201248 so it would be great to see some more testing :)

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community

yched asks if it's better to have the array key defined rather then not defined? It doesn't really matter because Drupal just loops through them all.

This looks good! Just tested it during install as well as in the Clean URLs check page. A couple more tests in other systems would be good too.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Automatically add Drupal.settings.base_path » Automatically add Drupal.settings.basePath
Status: Reviewed & tested by the community » Needs review
FileSize
base_path.patch2.56 KB

Renamed "base_path" to "basePath" (so the end result is Drupal.settings.basePath) for consistency with the rest of the JS.

I prefer my version, because otherwise you have the same thing there twice, which might cause confusion. ("Which one really affects the name of the setting?")

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community

Whatever floats your boat! We're going to also have to add this to Converting 5.x modules to 6.x.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

After it is committed ;) :)

yched’s picture

yched CreditAttribution: yched commented

Tested, works perfectly for me too. +1 for RTBC.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Looks good, thanks, committed.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Yay :) Documentation is already updated: http://drupal.org/node/114774#base_path.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

I didn't participate in this issue, but great job everyone! Thanks for making it basePath also. Now I can take basePath out of like 5 of my modules :D

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.