Make WordPress Core

Opened 13 years ago

Last modified 5 years ago

#17763 new enhancement

comments_popup_link() need a get_* version

Reported by: dd32's profile dd32 Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.2
Component: Comments Keywords: needs-patch
Focuses: template Cc:

Description

Currently comments_popup_link($zero = false, $one = false, $more = false, $css_class = '', $none = false) has no get_*() version.

Usage situation: Where the link needs to be used within a larger string being concatenated.

Attachments (6)

17763.diff (5.4 KB) - added by kawauso 13 years ago.
17763.2.diff (6.1 KB) - added by voldemortensen 9 years ago.
17763.3.diff (6.1 KB) - added by voldemortensen 9 years ago.
17763.4.diff (6.1 KB) - added by voldemortensen 9 years ago.
17763.5.diff (6.1 KB) - added by voldemortensen 9 years ago.
fixed get_comments_number to get_comments_number_text
17763.patch (8.6 KB) - added by kevdotbadger 9 years ago.

Download all attachments as: .zip

Change History (29)

#1 @kawauso
13 years ago

  • Keywords has-patch added; needs-patch removed

The following patch:

  • Introduces get_comments_popup_link()
  • Drops @return null Returns null on single posts and pages (this never worked as no other values were ever returned)
  • Changes most @param to bool|string
  • Adds $echo parameter to comments_number() (get_comments_number() returns the raw number, not the template used by comments_number())

@kawauso
13 years ago

#2 @MeanderingCode
13 years ago

I would like to make a case for adding a piece of functionality related to post_types and comment support:

If a post type does not have comment support, nothing should be output instead of a "Comments Off" message.

This would require:

  • Checking comments support for $post->post_type, return empty string or null if no comments support
  • Changing logic in twenty-ten/loop.php before comments_popup_link() is called to determine printing the | separator

Am I supported in thinking this makes sense?

#3 @goto10
12 years ago

  • Cc dromsey@… added

#4 follow-up: @leblogdudeveloppeur
12 years ago

  • Cc leblogdudeveloppeur added

Why not close this ticket now if the original problem is corrected by the patch ? For me the comment of MeanderingCode is the topic of a new ticket as an evolution.

The original topic is a real problem for internationalization because it is not easy (except if I rewrite the implementation without echo) to put the result of the function in a printf(). It means that we have things like this :

<div><?php printf(__('Published on: <span>%1$s</span> by <span>%2$s</span> - ', 'azsimple'), get_the_time($formatTime), get_the_author()); ?><?php comments_popup_link(__('Leave a Comment', 'azsimple'), __('1 Comment', 'azsimple'), __('% Comments', 'azsimple')); ?></div>

... in templates. If the translator want put the comment link before "Published on...", he can't with just *.po files.

#5 in reply to: ↑ 4 @SergeyBiryukov
12 years ago

Replying to leblogdudeveloppeur:

Why not close this ticket now if the original problem is corrected by the patch ?

The patch still needs to be reviewed and committed.

#6 @pauldewouters
12 years ago

  • Cc pauldewouters added

#7 @nacin
10 years ago

  • Component changed from Template to Comments
  • Focuses template added

#8 @palpatine1976
10 years ago

+1

Would love to see this added to core so that I don't have to include copy/edited/pasted custom functions or patch WP core in future projects. The patch was submitted 3 years ago, no?

#9 @dgwyer
9 years ago

+1 on getting this into core. Would be very useful when string building the post meta.

#10 @boonebgorges
9 years ago

  • Keywords needs-refresh good-first-bug added; has-patch removed

#11 @voldemortensen
9 years ago

  • Keywords has-patch needs-testing added; needs-refresh removed

I believe I got everything in 17763.2.diff. Needs testing.

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


9 years ago

#13 @voldemortensen
9 years ago

Updated a line after talking with obenland. Now checking with DrewAPicture to make sure the docs are correct.

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


9 years ago

#15 @voldemortensen
9 years ago

Fixed to match code and documentation standards.

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


9 years ago

#17 @toscho
9 years ago

There is no need to instantiate the variable with an empty string:

        $output = '';

        $output .= '<a href="';

This could be reduced to:

        $output = '<a href="';

@voldemortensen
9 years ago

fixed get_comments_number to get_comments_number_text

#18 @Yahire Furniture
9 years ago

+1. Is this still being worked on? I need this very feature for one of our websites? If yes any idea when it will be included?

@kevdotbadger
9 years ago

#19 @kevdotbadger
9 years ago

  • Keywords dev-feedback added

I took @voldemortensen's code and did some very minor housekeeping on it, as well as making @toscho's amend.

It works for me, but needs more testing.

Last edited 9 years ago by kevdotbadger (previous) (diff)

#20 follow-up: @SergeyBiryukov
9 years ago

  • Keywords good-first-bug removed

comments_popup_link() has significant i18n issues, see #13651.

Instead of introducing a get_*() version with the same issues, could we introduce a new function, more i18n-friendly (accepting proper plural forms instead of a pseudo-plural '% Comments' string), and with a more relevant name?

#21 in reply to: ↑ 20 @DrewAPicture
9 years ago

Replying to SergeyBiryukov:

comments_popup_link() has significant i18n issues, see #13651.

Instead of introducing a get_*() version with the same issues, could we introduce a new function, more i18n-friendly (accepting proper plural forms instead of a pseudo-plural '% Comments' string), and with a more relevant name?

+1 for a new approach.

#22 @rachelbaker
9 years ago

  • Keywords needs-refresh added; dev-feedback removed

Needs a refreshed patch based on the input from @SergeyBiryukov

#23 @boonebgorges
8 years ago

  • Keywords needs-patch added; has-patch needs-testing needs-refresh removed
Note: See TracTickets for help on using tickets.