Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
Issue tags: +Needs backport to D6, +Needs backport to D7
FileSize
787 bytes
jhodgdon’s picture

Status: Needs review » Needs work

I don't think that description really covers what the 'text' variable is used for?

   if (isset($titles[$text])) {
      $attributes['title'] = $titles[$text];
    }
    elseif (is_numeric($text)) {
      $attributes['title'] = t('Go to page @number', array('@number' => $text));
    }
  }

  return l($text, $_GET['q'], array('attributes' => $attributes, 'query' => $query));
jhodgdon’s picture

Title: Documentation problem with theme_pager_link » theme_pager_link doc missing 'text' variable
jhodgdon’s picture

droplet’s picture

Status: Needs work » Needs review
FileSize
854 bytes
franz’s picture

Status: Needs review » Needs work

I think you need to wrap comments in 80-width lines, right?

jhodgdon’s picture

Yes, 80-character wrapping.

Also, I still don't think that description is correct. Please look at the whole function code. $attributes is a local variable in the function, so it shouldn't be mentioned in the doc. Or if you need help figuring it out, add a note and someone will I'm sure help.

franz’s picture

I just noticed the information was not right.

If $attributes['title'] is empty, it just uses the $text to determine whether to use a predefined title (e.g. "Go to first page"), a title concerning page number (e.g. "Go to page number @number"), or just no title. However, it doesn't ever set the title to be $text.

franz’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

That's better... the description has a typo in it ( $variables['atributtes']['title'] ), and I think it could use a little more detail. How about something like this:

The link text. Also used to figure out the title attribute of the link, if it is not provided in $variables['attributes']['title']; in this case, $variables['text'] must be one of the standard pager link text strings that would be generated by the pager theme functions, such as a number or t('<< first').

One other thing I just noticed for the next patch. In the 'attributes' item, could you change:
apply to a pager anchor tag.
to
apply to the pager link.

Thanks!

franz’s picture

Status: Needs work » Needs review
FileSize
1.41 KB

I liked that version.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks! 8.x/7.x... then mark backport to 6.x please.

droplet’s picture

Status: Reviewed & tested by the community » Needs work

It seems not right.

<< first should be « first

  // Set each pager link title
  if (!isset($attributes['title'])) {
    static $titles = NULL;
    if (!isset($titles)) {
      $titles = array(
        t('« first') => t('Go to first page'), 
        t('‹ previous') => t('Go to previous page'), 
        t('next ›') => t('Go to next page'), 
        t('last »') => t('Go to last page'),
      );
    }
    if (isset($titles[$text])) {
      $attributes['title'] = $titles[$text];
    }
    elseif (is_numeric($text)) {
      $attributes['title'] = t('Go to page @number', array('@number' => $text));
    }
  }

would be generated by the pager theme functions

If $text isn't a numeric and not in the array. it doesn't nothing.

Point them to theme_pager function would get more clear ideas.

franz’s picture

Status: Needs work » Needs review
FileSize
1.44 KB

If $text isn't a numeric and not in the array. it doesn't nothing.

Meaning? It does not use it for the title attribute only. I think the current explanation pretty much covers this.

I added a see also statement and fixed the symbol for '« first', nice catch

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Agreed, the explanation does cover it by saying "in this case", meaning if it is going to figure out the title attribute. Nice catch on the << vs. special character, yes. 8.x/7.x... then mark backport to 6.x please.

webchick’s picture

Version: 8.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs backport to D7

Committed and pushed to 8.x and 7.x. Thanks!

Moving back to 6.x.

franz’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.16 KB

Ported.

droplet’s picture

Status: Needs review » Needs work

no $variables like Parameters in D6

franz’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

My bad.

jhodgdon’s picture

Status: Needs review » Needs work

Please read the text at below the Upload field -- don't name your patches with the _D6 extension.

Review of latest patch:

Can you also fix the first line of the function doc so it conforms to standards?
http://drupal.org/node/1354#themeable (or see the D7 one-line summary)

Other than that, looks good.

franz’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

What's wrong with the first line, really? Re-submitting D6 patch...

jhodgdon’s picture

Status: Needs review » Needs work

Did you look at the link I posted for standards on what the first line should be for themeable functions? The existing first line on this function doc does not follow the standard.

franz’s picture

You mean "Returns HTML for a" ? That doesn't look mandatory, specially because this has been lying around on D6 for a long time already...

jhodgdon’s picture

Everything on that page is mandatory - it is the standards page, not the suggestions page. :)

That doesn't mean that the standards are currently being followed by all docs (especially in D6), but when we fix docs for a particular function, we try to bring that function's doc up to standards.

franz’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

Then it is better to have all of the other functions corrected as well to keep consistency. =)

jhodgdon’s picture

Status: Needs review » Needs work

If you are going to fix all of them (which is a nice idea), we will need to bump this issue back up to Drupal 7.x/8.x, since they are likely also wrong there. The other option (recommended) is to leave this issue as D6 and fix only the function in question.

Also: Return -> Returns. You got the first one right, but not the rest.

franz’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

D7/8 already contains this fix on docs.

Fixed spelling

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

you're right, it does! Sorry about that.

OK, looks good for Drupal 6 -- brings the D6 doc for these functions up to the same standard as Drupal 7/8, and also adds the missing 'text' argument to the one function. Thanks!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed, pushed.

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