Expand theme_pager_link() so it can accept html input or specify invisible elements

Tagging

Tagging

Related #1778990: Merge theme_pager_link*() theme functions into theme_link()

Well, we currently use check_plain() as a matter of security. That way you can do

theme_pager_link(array(
  'text' => '<script>alert();</script>',
  ...
);

But the JavaScript is actually not executed.

The function theme_link() supports $variables['options']['html'], I think we should do the same here. See http://api.drupal.org/api/drupal/core!includes!theme.inc/function/theme_...

Note that because of the above, this would automatically be fixed by #1778990: Merge theme_pager_link*() theme functions into theme_link(), so I don't know if this should simply be marked duplicate.

The change was simple as another patch had already replace the l() function. I when for removing check_plain as that is how l() implements the html option. The new patch has added $variables['options']['html'].

Looking at the logic in theme_pager_link() I don't think it can be merged into theme_link(), however the end of theme_pager_link could use theme_link instead of repeating the build of the html output (but that isn't part of this issue).

Looks great! Some minor remarks:

+++ b/core/includes/pager.inc
@@ -428,6 +428,12 @@ function theme_pager_last($variables) {
+ *     - 'html' (default FALSE): Whether $text is HTML or just plain-text. For
+ *       example, to include an image tag into the link, this must be set to TRUE, or
+ *       you will see the escaped HTML image tag. $text is not sanitized if
+ *       'html' is TRUE. The calling function must ensure that $text is already
+ *       safe.

Instead of the "(default FALSE):", I think we usually do a "Defaults to FALSE." at the very end. Also we should be referencing "$variables['text']" and not "$text", because the variable in the PHPDoc is $variable. I'm not sure we need the explanation with the image tag. The (currently) last sentence is very important, though!

+++ b/core/includes/pager.inc
@@ -439,6 +445,7 @@ function theme_pager_link($variables) {
+  $html_option = isset($variables['options']['html']) ? $variables['options']['html'] : FALSE;

@@ -478,7 +485,7 @@ function theme_pager_link($variables) {
+  return '<a' . new Attribute($attributes) . '>' . ($html_option ? $text : check_plain($text)) . '</a>';

In theme_link() we check $variables['options']['directly'] without setting another variable. I think it would be most consistent to declare $options up top, and then reference $options['html'] below.

Also the patch needs to update drupal_common_theme() to reference the new parameter.

Marking needs work.

Done, I have also added options to the other pager link theming functions (otherwise it isn't much use):

• theme_pager_last
• theme_pager_next
• theme_pager_previous
• theme_pager_first

That looks great. I had forgotten about the other pager theme functions, nice one! I was about to mark this RTBC, but I thought maybe we should have some tests for this.

I would think adding a new function (something like testHTMLTest or whatever) to PagerTest would be enough. That could probably use assertThemeOutput().

Slightly related: #1598886: Clean up pager theme functions

I have added a test for theme_pager_link.

Nice work on the tests. Great job bringing a < blink > tag into the core codebase! :-)

I think after some minor comment touch-ups (and the fixing of the tests), we can call this done:

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.php
@@ -168,4 +168,43 @@ protected function assertNoClass(SimpleXMLElement $element, $class, $message = N
+   * Tests html and text in the pager link title.

Perhaps: "Tests HTML and plain text in the pager link title."

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.php
@@ -168,4 +168,43 @@ protected function assertNoClass(SimpleXMLElement $element, $class, $message = N
+    // Check with plain text as plain text.
...
+    // Check with html as html.
...
+    // Check with html as plain text.

1. "html" should be "HTML".
2. This could use a little longer description for clarity. Maybe something like (feel free to improve on this): "Test plain text as pager text with HTML input disabled.", "Test HTML as pager text with HTML input enabled.", "Test that HTML as pager text with HTML input disabled gets stripped."

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.php
@@ -168,4 +168,43 @@ protected function assertNoClass(SimpleXMLElement $element, $class, $message = N
+      'page_new' => array( 1 ),
...
+      'options' => array( 'html' => FALSE )
...
+      'page_new' => array( 1 ),
...
+      'options' => array( 'html' => TRUE )
...
+      'page_new' => array( 1 ),
...
+      'options' => array( 'html' => FALSE )

There should be no spaces after the opening and before the closing parenthesis.

Can't check right now why the tests are failing.

Regarding the tests:
1. It seems you need to do 'text' => 'next >', instead of 'text' => 'next' to get the 'title' attribute.
2. You need to dynamically figure out the 'href' attribute. Probably use url() for that.

Sorry, but I found something to complain again... :-)

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.php
@@ -168,4 +169,55 @@ protected function assertNoClass(SimpleXMLElement $element, $class, $message = N
+    // Figure out the URL that the pager will generated.

Typo: generated -> generate

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.php
@@ -168,4 +169,55 @@ protected function assertNoClass(SimpleXMLElement $element, $class, $message = N
+    $query = &drupal_static(__FUNCTION__);
+//    if (!isset($query)) {
+      $query = drupal_get_query_parameters($_GET, array('page'));
+//    }

I don't think we need a static here, this function should only be called once.

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.php
@@ -168,4 +169,55 @@ protected function assertNoClass(SimpleXMLElement $element, $class, $message = N
+    $output = theme('pager_link', $next_variables);
+    $expect = strpos($output, '>next</a>') !== FALSE;
+    $this->assert($expect, $expect ? t('Pager plain text title is correct') : t('Unexpected pager plain text title output @output.', array('@output' => $output)));

Hmm... I guess since we're really only interested in the title and the pager markup is tested elsewhere, it may be a good idea to not use assertThemeOutput() here, but only assert on the title (not sure yet...). If so, we should not use assert() here directly, rather assertTrue(strpos($output, '>next') !== FALSE). The two different assert messages really are non-standard in core. This also goes for the other two assertions of course.

I have removed generating the URL as it isn't needed any more.
I have also removed the two different assert messages.

That looks great! Thanks for sticking with this.

Would be great if we'd do #1598886: Clean up pager theme functions first -- that one has been ready for a long time already, and committing it first will only result in diff hunks to remove here.

I'd volunteer to re-roll that one in case this gets in first. Should be equally trivial.

#20: theme_pager_link-1805678-20.patch queued for re-testing.

Given where #1778990: Merge theme_pager_link*() theme functions into theme_link() is heading, I wonder whether it makes sense to do this.

Actually, I just "merged" the needed change into that.

#20: theme_pager_link-1805678-20.patch queued for re-testing.

Can this issue be closed as as duplicate?

I guess it probably makes sense, your call though, @wdm, you worked on this. We should still port the tests from here to that issue, though.