The url for the link is being given as url(admin/content). From what I can understand, there is no need to use url() when the more_link theme function uses l() to create the link.

This patch passes $base_path . "admin/content" to the theme function.

CommentFileSizeAuthor
node-more-link.patch763 bytesjames.elliott

Comments

ksenzee’s picture

ksenzee
she/her
commented
Status: Needs review » Reviewed & tested by the community

Yeah, no need for double url()ing. Looks good.

dries’s picture

dries commented

At first glance, there are more instances of this:

modules/aggregator/aggregator.module:          $read_more = theme('more_link', array('url' => url('aggregator/sources/' . $feed->fid), 'title' => t("View this feed's recent news.")));
modules/aggregator/aggregator.module:          $read_more = theme('more_link', array('url' => url('aggregator/categories/' . $category->cid), 'title' => t("View this category's recent news.")));
modules/node/node.module:      $output .= theme('more_link', array('url' => url('admin/content'), 'title' => t('Show more content')));
modules/simpletest/tests/theme_test.module:  $GLOBALS['theme_test_output'] = theme('more_link', array('url' => url('user'), 'title' => 'Themed output generated in hook_init()'));
effulgentsia’s picture

effulgentsia commented
Status: Reviewed & tested by the community » Closed (duplicate)

This is indeed a bug, because double url() if the first url() doesn't use 'absolute' => TRUE does lead to problems. For example, if base_path() is '/mysubfolder/', then url(url('admin/content')) is '/mysubfolder//mysubfolder/admin/content'.

This bug is a regression caused by #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain(), where we changed theme_more_link() and related functions to call l() instead of hard-coding their own anchor tags (as part of fixing the double check_plain() caused by that hard-coding).

The patch isn't the right fix though for two reasons:
1) Since theme_more_link() calls l(), if you're passing a non-absolute URL to it, then you need to pass a Drupal path (e.g., 'admin/content') rather than a partial URL (e.g., base_path() . 'admin/content').
2) I'm concerned about setting a variable called 'url' to a Drupal path. For security, we need people to know when to pass a path, when to pass a URL, and when to pass something that might be either. See #845000: Inconsistency and lack of clarity for which functions are responsible for stripping dangerous URL protocols.

Because of the latter point, I'm marking this a duplicate of that issue: that issue will need to deal with this. I'll add this issue as a link on that one.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/node/node.module	19 Jul 2010 21:52:22 -0000
@@ -2159,7 +2159,7 @@ function theme_node_recent_block($variab
-      $output .= theme('more_link', array('url' => url('admin/content'), 'title' => t('Show more content')));
+      $output .= theme('more_link', array('url' => $base_path . 'admin/content', 'title' => t('Show more content')));

I want to amend that the situations, in which you manually have to fiddle with $base_path, except perhaps in theme templates but also not really there, are very very rare.

All red warning lights should have tilted the <blink> tag in light of this diff line. :)

Extra neon lights for:

$base_path is a global variable that needs to be put into local scope first. However, it should not be accessed directly, use base_path() instead.

Powered by Dreditor.