l() and url() each have a long list of arguments that are used in various situations. When you need to set one of the last arguments, you have to include all the previous ones, which results in lovely code such $channel['link'] = url("blog/$uid", NULL, NULL, TRUE);.

This patch changes l and url to follow the following signatures:

function l($text, $path, $options = array());
function url($path = NULL, $options = array());

Options is an associative array that contains 'query', 'fragment', 'absolute', 'alias' for both, and 'html' and 'attributes' for l(). I chose this syntax because the majority of l() and url() calls consist of only $path and $text.

'Alias' is a useful new ability (which is required in chx's menu changes) where you can prevent the expensive alias lookup in url() if you already have the alias.

I converted all of core using a replacement script, though it seems to get confused somewhere in system.module and I had to manually fix 3 links. The rest of core seems to work okay though.

CommentFileSizeAuthor
#36 refactor-l-url_3.patch61.19 KBSteven
#24 url_refactor_2.patch58.73 KBkilles@www.drop.org
#22 url_refactor_1.patch57.88 KBkilles@www.drop.org
#21 url_refactor_0.patch57.52 KBkilles@www.drop.org
#20 url_refactor.patch58.05 KBkilles@www.drop.org
#18 refactor-l-url_2.patch58.48 KBSteven
#16 refactor-l-url_1.patch58.56 KBSteven
#15 replace.php_.txt3.73 KBSteven
#14 refactor-l-url_0.patch58.6 KBSteven
#1 replace.php.txt3.46 KBSteven
refactor-l-url.patch51.46 KBSteven

Comments

Steven’s picture

Steven commented
StatusFileSize
new replace.php.txt3.46 KB

Here is the updating script. Note, do NOT run twice on the same code.

It updates all .inc, .module and .php files under the current working directory (and subdirectories).

kkaefer’s picture

kkaefer commented

+1 on this. The parameter definitions of url() and l() are really annoying.

I applied the patch, clicked through my Drupal installation and didn’t spot any errors on the first glance.

merlinofchaos’s picture

merlinofchaos commented

Big +1 on this. l() and url() syntax is highly irritating.

chx’s picture

chx commented

What else could be said than I totally admire and love this piece of code?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Excellent. This is just the type of change that makes the API easier and more intuitive to use.

A couple questions about the patch:
1. The PHPDoc specifies the default for absolute and alias and not for query and fragment. Should this be mentioned as well?
2. Where is 'alias' actually used in the code? I read it a couple times and couldn't figure this out.
3. $options += array(
won't that cause problems under PHP 5? I thought it should be array_merge.

Will try and test this later to confirm this.

kkaefer’s picture

kkaefer commented

@webchick: afaik, += for array only adds the values in the second array to the first array if the key is not present, thus "filling up" unset values in the $options array.

Steven’s picture

Steven commented

webchick: Isn't it rather obvious that these string arguments default to empty?
Also, the 'alias' property is used normally in url(), not sure why you missed it. A simple cmd-f on the patch should've brought up:

  if (!empty($path) && $path != '<front>') {
    if (!$options['alias']) {
      $path = drupal_get_path_alias($path);
    }
Steven’s picture

Steven commented

We use + for arrays in form API too. It is faster than array_merge, because it does not renumber numerical keys.

dries’s picture

dries commented

I'm OK with this patch but it's a little bit awkward to put the query string in an array.

1. It would be good to know how often we need to pass an array. If we have a pass an array 70% of the time, it might actually be harder to use?

2. url() and l() are performance critical functions. Certain pages have hundreds of URLs. I think we'll want to benchmark this patch so we can consider the performance implications (if any).

Steven’s picture

Steven commented

1. It would be good to know how often we need to pass an array. If we have a pass an array 70% of the time, it might actually be harder to use?

The vast majority of these calls in core only has the 2 parameters (roughly 75%). For the other calls, there is no real preference for any specific parameter. We often pass only a fragment, only a query, only attrbutes, etc. The new syntax is much easier to remember and read in this case, rather than having to remember and count the arguments, you can just tell by the array key.

Performance-wise, someone else will have to benchmark.

ianivs’s picture

ianivs commented

Another option that would be useful is 'protocol'. Right now, if I want to create a bunch of https links I have to process them after l(). There are many other situations where using something other than 'http' is needed.

Like chx said: "and without this patch, adding another parameter [to l()] is crazy". So it may be a good time to think about it.

rkerr’s picture

rkerr commented

Sweet.

If we can get "scheme" or "protocol" support I'll +1 :)

Also, it's good keeping query string values in an array as it gives us a hope of transitioning to "suggested" practice of using ; instead of & as a separator without worrying module developers.

andremolnar’s picture

andremolnar commented

For every second saved by not having to type a couple of parameters I lose a minute looking up what possible values could go into that array and in what format they are expected. This is of course because I use an IDE that tells me what arguments are expected when I start typing a function name. And $options = array is just about useless to people as a hint for what they should include next.
But nobody cares about the IDE users - they're obviously not hard core.
Why not just order the arguments from most commonly used to least commonly used.

Steven’s picture

Steven commented
StatusFileSize
new refactor-l-url_0.patch58.6 KB

The point about ordering is that the arguments are each used as frequently as eachother. There is no optimal order.

Instead of seeing:
url("user/$account->uid", NULL, NULL, TRUE)
you get:
url("user/$account->uid", array('absolute' => TRUE)).

I don't need to figure out what the TRUE is, I can see it immediately. IDE or not. The available values to l() and url() are pretty logical, as you are dealing with a URL (query, fragment, absolute, ...) and the functionality of these functions remains the same as before.

Updated patch attached:

  • I'm convinced the calls missed by replace.php are due to a PCRE bug. If I move the missed lines of code higher up in the file, they are converted correctly. I added a workaround which processes the file in chunks rather than all at once. The usage of recursive regular expressions might have something to do with this.
  • I made the code in theme_links() much shorter, as $links now shares the same format with l() (mostly).
  • I also included a very subtle but IMO important improvement to both l() and url(). 'query' can now be passed in as an array, as well as a "key=value&key=value" string. This means you can pass in raw data, and it will be automatically urlencoded and formatted with drupal_query_string_encode(). This is another defense against security issues (although it is 100% backwards-compatible).

Patch includes the l/url() change as well as the result of applying the replace.php script to core.

Steven’s picture

Steven commented
StatusFileSize
new replace.php_.txt3.73 KB

Updated replacement script. Do NOT run twice on the same code.

Steven’s picture

Steven commented
StatusFileSize
new refactor-l-url_1.patch58.56 KB

Adding in scheme is not so easy, because it would only work for absolute=>TRUE and we'd have to rewrite the base_url to do it. It might slow down the patch. However, if we do add it, it would be available for all l(), url() calls, as well as anything added via hook_links(). This array notation is quite flexible and can bubble through all of Drupal.

I forgot some debug code though, and removed the duplicate adding of default values between l() and url(). New patch attached.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Patch no longer applies.

Steven’s picture

Steven commented
Status: Needs work » Needs review
StatusFileSize
new refactor-l-url_2.patch58.48 KB

Killes did some testing with a 5.0 version of this patch, and reported a speed-up of roughly 10-15% (according to m3avrck).

This may sound weird given the added complexity, but we are passing in a lot less arguments every time. Each call to the old l() had to fill in up to 5 default values for arguments (+ 2 required), and passed 4 of them to url(). With the new code, there is only 1 default value and it is filled in in one go (with the += operator). Also, the variables for url() are only filled in inside url() and do not have to be passed from l() to url().

This makes sense if you consider that the majority of all calls to l() and url() don't use any additional arguments (simple links, with no query/fragment/etc), so most calls had a lot of overhead before to fill in those arguments and pass them along needlessly.

I trust killes' benchmarking fu and though I am surprised at the result, it would not be the first time that we see something like this in PHP.

Rerolled for HEAD.

dries’s picture

dries commented

On my benchmark setup, I observe a minor performance degradation. Generating the main page is, on average, 0.4% slower. Pretty insignificant so that's a good thing.

Would still like to know what killes/maverck did to get these results?

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new url_refactor.patch58.05 KB

patch re-rolled

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new url_refactor_0.patch57.52 KB

oops...

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new url_refactor_1.patch57.88 KB

there was one link in the commment form not converted.

killes@www.drop.org’s picture

killes@www.drop.org commented

Updated patch.

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new url_refactor_2.patch58.73 KB

the patch...

killes@www.drop.org’s picture

killes@www.drop.org commented

I don't know why Steven thought I had done a benchmark. I had wanted to, but actually did not. But I have now. The result is:

With patch:

374+-7 s

Without patch

365+-5 s

That is, the patch makes my test take about 2.5 % longer.

m3avrck’s picture

m3avrck commented

Hmmm, I must have gotten something backwards, but I could have sworn that killes said he did benchmarking and it was faster, mentioned in IRC... that's when we were all scratching our heads as to why that was possible.

Perhaps that was a fluke, doh!

moonray’s picture

moonray commented

If we had all our links in drupal in one consistent format, that we could just drop into url() or l() or another similar function it would make modding much easier. (see 191650)

It would be nice to be able to drop such an array, as gotten from the links used in hook_link() and hook_link_alter(), into one of these functions. But perhaps a new function is needed instead?

Thoughts?

chx’s picture

chx commented

2.5% -- but let's not forget that now we can pre-store aliased links and show those. This would provide a much bigger win than 2.5% . Of course, we could just add an 8th parameter to l... but I would really prefer not.

dries’s picture

dries commented

2.5% -- but let's not forget that now we can pre-store aliased links and show those. This would provide a much bigger win than 2.5%

Agreed. We pay some, but we might amortize that cost by being able to pass path aliases into l().

However, I'd like to see proof of that. Are we sure we'll get that 2.5% back? How are we going to do that? This patch would be a no-brainer if someone extended this patch to do exactly that. Until then, we'll have to assume that this patch degrades performance a little bit ...

In other words, bonus point if someone can take advantage of this patch to show us that it is the right thing to do. :)

Steven’s picture

Steven commented
Status: Needs review » Reviewed & tested by the community

AFAIK it's been shown before that preloading the alias can have a significant speed up for heavily aliased sites. Regardless, the addition of this check if just one 'if' statement in what is already a complicated function. I doubt the alias feature would cause any changes in performance.

Everyone agrees the new format is easier to use and the performance hit turns out to be not that bad. The fact that we now have a unified link format to use across Drupal is good too and it makes code more readable.

Can this please be committed?

dries’s picture

dries commented

Everyone agrees the new format is easier to use and the performance hit turns out to be not that bad.

No, some people voted against this patch because it doesn't play nice with IDEs that have auto-completion.

AFAIK it's been shown before that preloading the alias can have a significant speed up for heavily aliased sites.

Thus, to make a long story short: the key advantage of this patch is the 'alias' option IMO. I'd like to know that it actually does buy us something -- we have no proof of that. In fact, Gerhard tried to take advantage of this patch to remove some alias lookups but it didn't buy us anything.

Steven’s picture

Steven commented

Um, the IDE argument is not valid IMO. The new syntax is so simple, you don't need autocompletion anymore. Plus, autocompletion only helps when writing code. It does not help when reading code.

As for the 'alias' thing, this patch was created because adding another '$alias' parameter to l() and url() was the drop that spilled the bucket. But even without 'alias', this patch has a lot of merit... the alias feature is only a single if statement that can easily be removed.

chx’s picture

chx commented

the menu system will use alias. that could save quite a number of queries. It was raised more than once that we could store links for a node... all the possibilities :)

Jaza’s picture

Jaza commented

Some issues with the doxygen comments in this patch:

  1. Why do l() and url() now have this doxygen comment about the 'query' option:

    A query string to append to the link, or an array of query key/value properties.

    The comment in the current code says nothing about an array of key/value properties. Neither the current nor the patched code caters to an array of key/value properties. Also, I think that accepting two different formats (string or array) would be a bad idea, as it would only make for inconsistency and for a messier API. Plus, I can't see any code in core that actually passes in an array for 'query' (hardly surprising if url() can't actually handle it!).

  2. The doxygen comment about the 'fragment' option should include something about the '#' (pound) symbol, e.g:

    A fragment identifier (named anchor) to append to the link. The identifier should not begin with a pound (#) symbol, as this will be prepended automatically.

Other than that, looks good to me. If the benchmarks show a 2.5% performance hit, that's acceptable IMO, considering the improved readability/writability of code that this patch brings in.

dries’s picture

dries commented

Alright. Let's get the documentation fixed, and then it's ready to go.

Steven’s picture

Steven commented
StatusFileSize
new refactor-l-url_3.patch61.19 KB

Ack, the array query syntax got lost in #16... I must have messed up the merge.

I disagree that this syntax is messy: it follows with other Drupal practices like passing MIME headers in as arrays or keeping links in a structured format rather than an HTML string. Plus, this is important because often people forget to call urlencode() on URL arguments, which can lead to bugs, header spoofing or even XSS problems. By passing in an array, Drupal automatically urlencodes for you.

// Change:
+  if (is_array($options['query'])) {
+    $options['query'] = drupal_query_string_encode($options['query']);
+   }

// Before:
url('foo', 'title', 'key='. urlencode($myvar) .'&foo='. urlencode($myothervar));

// After:
url('foo', 'title', array('query' => array('key' => $myvar, 'foo' => $myothervar)));

(note that the old syntax still works and has some uses too, but the other method is recommended when you control the entire query string).

I rerolled the patch with the query array syntax and added the note about not including '#' for fragments.

The examples I can find in core don't benefit from the new syntax, as they either just use drupal_get_destination() or append a piece of existing query string. For the latter, we could use drupal_query_string_encode() manually instead of building the string directly.

That doesn't mean it can't be used to clean up contrib modules though. Plenty of them use straight GET arguments. If we're going to clean up l() and url(), we might as well remove all icky bits. Query arguments are structurally key/value pairs, and we should support handling this data in the most natural form (an array) rather than only in serialized strings.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I kind of lean toward ditching the old syntax altogether and just using arrays. You're right; it is intuitive that way.

Having to manually escape arguments seems like a dangerous thing to trust developers to do. But you said that there were use cases for this?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

The examples I can find in core don't benefit from the new syntax, as they either just use drupal_get_destination() or append a piece of existing query string.

Sigh. I need to not respond to issues before noon. ;)

dries’s picture

dries commented
Status: Reviewed & tested by the community » Active

Committed to CVS HEAD. Marking this 'active' until it has been properly documented in the upgrade instructions. :)

syscrusher’s picture

syscrusher commented

Nice work, Dries! I use these functions a lot in the Links Package (contrib module set). You've caused me to need to rework a little of my code, but I'm not complaining because I think this is a good change to the API. This will make it much more intuitive for future development, because we don't have to remember the order of all those extra parameters since the array is associative.

I know it's kind of after-the-fact, but I'm giving it a +1 anyway just to say thanks!

Syscrusher

dww’s picture

dww
we/he/they
commented
Status: Active » Fixed

http://drupal.org/node/114774#url

Anonymous’s picture

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