Add wrapper function for current url (i.e., $_GET['q'])

I've seen this come up repeatedly as a DrupalWTF, so I would like to see this fixed, too. The question is, what to call this function so people will actually find it?

We kicked around some about naming conventions in #drupal. Since $base_url refers to something with http://... in it, current_url() would probably be confusing if it only referred to the 'node/12' part. Quicksketch suggested current_path instead, since elsewhere we refer to something like node/12 as a path. I like that better, for sure.

Adding the appropriate tags. ;)

The coding standards (know them, love them, memorize them, and buy them a nice Easter dinner to tell them how much you appreciate their awesomeness) dictate indentation should be 2 spaces rather than tabs.

Additionally, I think the PHPDoc is not quite sufficient to explain to someone who was paging through the huge list of functions how they would know that this was the one that did what they want. How about adding an example?

The corresponding setter is named menu_set_active_item(). We probably should harmonize the two.

I agree that menu_set_active_item() is an horrible name. I'm just calling for harmonization.

We don't have to harmonize right now in this issue. Thats just gonna drag down an otherwise useful patch.

Why would be bother with a tag called Bikeshed? Silly tags like this really do add rows to term_data and term_node. And then the parrots keep on re-using the tag everywhere this adding more rows and even generating email notificaitons. The silly tags make tag browsing harder and vocab admin harder. IMO, the benefit does not outweigh the cost.

Interesting idea. Subscribing.

Your example 2 is wrong, please note:

    $base_path = $parts['path'] . '/';

base_path always ends with a slash.

review in patch review sprint.... my first one.

documentation is not ver clear: current_path: Returns the current URL path of the page being viewed.
In a.d.o I found:
base_path(): Returns the base URL path of the Drupal installation. At the very least, this will always default to /.
$basepath: The base path of the drupal installation. At least will default to /.
$base_url: The base URL of the drupal installation.
As a novice, how can I tel the difference?

Should we have a global $current_path too ??

to make the full url i would think (as a novice): $base_url + base_path() + current_path() , but that is not working when drupal is installed in a subdirectory. $base_url + '/' + current_path() is the right way now.

I feel the function name is not clear: How do I know it is the URL path?; in phpinfo() it is REQUEST_URI.

Maybe rename base_path to base_dir; base_url is ok; current_path can be base_drupal_url ?

I am sorry that I have not posted this in the first round -- sorry that was a quick attempt to just CNW it and then return if the author still loves her/his patch. Apparently does, thanks!

So, the problem is with path aliases. If you have 'path/alias' as an alias for 'node/123' and you browse to example.com/drupal/path/alias then this function returns node/123 once you are past DRUPAL_BOOTSTRAP_PATH -- but hook_boot fires before that...

It is not different. $_GET['q'] is undocumented. And this function is documented. What I asked for is "simply" updating the documentation.

Note that I have moved the function to path.inc where it belongs and added comments about path aliases and bootstrap phases.

Some typo and syntax fixes :)

Syntax fix from chx, ready for commit I think ;)

Documentation awful good.
Simple 3 lines code, tested it (with devel PHP execute).
Simpletests passes.

I think: ready to commit.

see also #445152: Add wrapper function for current path alias ;)

Excellent patch! Throughout, I could find only code-style and commenting problems.

+ 
+/**
+ * Returns the current URL path of the page being viewed.
+ *

The first blank line in this chunk contains trailing white-space.

would return

The hypothetical form seems unnecessarily cumbersome here. "When viewing ..., this function returns ..." would be-- sorry, /is/ more straight-forward. I also think "return "node/306", while base_path() would return" sounds better than " and base_path() would return"...

+ * Example #3: When viewing "http://example.com/path/alias" and is a path
+ * alias for "http://example.com/node/306", then on viewing
+ * "http://example.com/path/alias", this function would still return
+ * "node/306".

Needs to be fixed so that grammar and redundancy are fixed. :)

+  public static function getInfo() {
+    return array (
+      'name'=>t('Path'),
+      'description'=>t('Test overall functionality of the path system.'),
+      'group'=>t('Path'),
+    );
+  }

Spaces required on either side of the "=>" operator.

+  /**
+   * Test integrity of current_path() function
+   */
+  function testCurrentPath() {
+    // Initialize series of comparisons between current_path and $_GET['q'] for various paths
+    $_GET['q'] = "/";
+    $this->assertEqual(current_path(), $_GET['q'], t('Test integrity of current_path() compared with backslash'));
+    $_GET['q'] = "";
+    $this->assertEqual(current_path(), $_GET['q'], t('Test integrity of current_path() compared with empty string'));
+    $_GET['q'] = "node/".rand(5, 15);
+    $this->assertEqual(current_path(), $_GET['q'], t('Test integrity of current_path() compared with random node path'));
+
+    // Generate a random string for comparison
+    $rand_str = "";
+    $possible = "0123456789bcdfghjkmnpqrstvwxyz";

1.) Comments need to end in a period and form complete sentences. (It couldn't hurt to do the same for the assertion messages, though I don't know if it's required there.)
2.) Strings are always encapsulated by single quotes, unless they contain variables, \n-escaped line-breaks, or apostrophes.
3.) String concatenations require a space on either side of the dot.

+      $char = substr($possible, mt_rand(0, strlen($possible)-1), 1);

The minus operator also needs spaces on either side.

+  }
+
+}
+

The first of these blank lines should be removed. I don't know about the second one.

we have $this->randomname for random strings. Probably just one test against a random string is enough?

Shouldn't all references to $_GET['q'] be converted to current_path() ?

If you do that in this or another patch, can sentences like these be converted without problem with the current patch?

common.inc

$path = isset($_GET['q']) ? $_GET['q'] : '';

language.inc:

$args = isset($_GET['q']) ? explode('/', $_GET['q']) : array();

Just one more quibble from me:

+ * Example #2: On viewing http://example.com/drupalfolder/node/306 this
+ * function returns "node/306" while base_path() returns "/drupalfolder/".
+ *
+ * Example #3: On viewing "http://example.com/path/alias" which is a path
+ * alias for "http://example.com/node/306", this function returns "node/306" 
+ * as opposed to the path alias.

You can either quote the URLs or not, but it should be consistent. :)

Okay, two quibbles:

+    $_GET['q'] = $this->randomName(); // Set $_GET['q'] to a random string for comparison.	

Comments should be on a separate line immediately before the code they reference.

That's really all I can see now.

+    // Set $_GET['q'] to a random string for comparison.	|
+    $_GET['q'] = $this->randomName(); |

Whoops, some whitespace left over at the end of these lines. But this is really the last thing!

This looks great now!

The new path.test will need some further test cases (for alias resolution and the like), but this part of it is ready, I believe.

A couple very minor things. Once they're fixed, please mark back to RTBC. mcrittenden, you've earned the title of "Trooper Of The Year." ;)

+class PathsTestCase extends DrupalWebTestCase {

This is missing a PHPDoc declaration on this class.

+  /**
+   * Implementation of getInfo().
+   */

Because this a) is not a hook, and b) is inherited from DrupalWebTestCase, we actually do not document it. Strange but true. So this chunk can be removed.

+    // Set $_GET['q'] to a random string for comparison.
+    $_GET['q'] = $this->randomName(); 
+    $this->assertEqual(current_path(), $_GET['q'], t('Test integrity of current_path() compared with random string path'));

The newly added $this->randomString(); is probably more appropriate here, as that's truly a random string, not a random set of garbled characters.

Oh, incidentally, if there is still some documentation somewhere telling you to do it like this, please correct it.

Since the remainder is really trivial and I have a moment, I've made the changes. Don't bother crediting me. ;)

Instead of duplicating content, I'm pointing to the same review remarks in #432864: Improve comments for function base_path(), which identically apply to this patch.

In general though, I'm a bit opposed to this patch. We have a lot of - senseless - function invocations to base_path() in core and contributed modules. Invoking a function just to get the value of a variable that is available directly is nonsense.

base_path() may be "more special" here, because it returns the value of a global variable. However, $_GET['q'] is... well... a standard environment variable. Accessing it via a function makes absolutely no sense to me. That's plain overhead.

So, if we add this function at all, then only as a helper function for n00bs that don't understand PHP. The PHPDoc should make that purpose clear, and that "regular" PHP coders do not need to resp. should not use it.

And - sorry, mcrittenden, your effort is great and this is not targeted to you...:

RTBC...? The tests are nonsense. These are testing PHP, not Drupal.

If at all, http://api.drupal.org/api/function/drupal_init_path/7 would have to be used for comparison.

No, it isn't senseless. This consolidates code so that we can make future changes in one place. Further, this makes code more readable. $_GET['q'] is a DrupalWTF that means nothing. current_path() is much more descriptive. And I don't really know where to begin with the suggestion that we add phpdoc that says that noobs should use this function but others should not.

The committers can decide whether this needs work or not.

Issues with PHPDoc and test comparison still remain.

I disagree completely that this is for "PHP n00bs." This is a DX issue. People with clean URLs enabled (which is the default) have absolutely no idea that there is a ?q=xxx in there, and unless they make a regular habit of var_dump($_GET);ing as they're developing (which, who does?) they end up in #drupal asking the question, "How do I get my current path?", which usually occurs about 3-4 times per week.

It's a nice convenience function for developers, with a negligible performance impact.

I do agree, however, that the PHPDoc of this function should be re-worked to match that of the committed base_path() function. That one was off my radar, so thanks for pointing that out.

And yeah, actually. The tests should be higher-level to cover possible future changes in how we determine what the current path is, as Moshe pointed out. So maybe something like drupalGet(''), check to make sure the current_path() returns ''. Create a node with a path alias, ensure that current_path() still returns node/$nid.

The point here is that the function itself is trivial and needs no exhaustive testing, but the path handling in total is completely lacking a test suite.

There are at this point two ways we can go:

1.) Decide that the function is trivial enough to need no test in itself, and leave the whole path.test for a separate issue.
2.) Decide to put in a trivial test case for this function.

I agree that we desperately need path.inc tests, and eg. check drupalGet('') -> '', or drupalGet('content/title-alias') -> 'node/123'. But if we do that here, this patch would lose focus from a trivial function addition and become a test suite for path.inc instead, and that should be avoided. Think of the kittens. ;)

Edit: Here we have a bit of a refactoring WTF, because obviously there is also path.module, and while alias resolution is actually done in path.inc, it is tested by modules/path/path.test, so the tests for path.inc don't need to duplicate that. But still, there are plenty of other parts of path.inc that do need tests.

I agree on removing the tests here (those serve no purpose).

+ * This function is not available in hook_boot; use $_GET['q'] instead. Also, be
+ * careful in the case of Example #3 if you use it in hook_boot as it
+ * will contain "path/alias". Calling drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH)
+ * makes this function available with a return value of "node/306".

When referring to function names and hooks, append trailing braces (hook_boot()).

The description is a bit contradictory - first you say it's not possible to invoke it in hook_boot(), then you explain the return value when used in hook_boot().

We should not teach developers to manually invoke drupal_bootstrap() during the bootstrap process. We already have some contrib modules that do this without any good reason.

Edit: nevermind. The change I wanted is incorporated in #60 already.

I think this is good. Its the role of hook_boot to tell people "this is not the hook you are looking for"...

Committed to CVS HEAD. Thanks. I've decided not to commit the tests, we can revisit that if desired.

Heads up from doing boost dev; i18n support with $_GET['q'] isn't all there because of http://api.drupal.org/api/function/drupal_init_path. Our solution was to use $_REQUEST['q']. This is an edge case, just thought I would let you guys know.

@mikeytown2: That has always been the case and probably won't change anytime soon. The language prefix is a "virtual" path component - prepended by url() when links are generated, removed by drupal_init_path() when $_GET['q'] is rebuilt. Virtual path prefixes only exist for the language system currently, but people (like me) already considered to re-use this approach to build site sections/contexts (think OG/Domain Access) by adding further prefixes to paths. Also note that $_REQUEST['q'] is always the unaltered 'q' query argument value, very potentially containing an URL alias, not an internal Drupal path. These are further reasons why $_GET['q']/$_REQUEST['q'] is almost unusable prior DRUPAL_BOOTSTRAP_PATH.