Too few arguments to sprintf in WP_List_Table::comments_bubble()

[nacin]

Not really sure where the bug is, but looks legit:

​http://wordpress.org/support/topic/warning-sprintf-functionsprintf-too-few-arguments?replies=1

[SergeyBiryukov]

Can reproduce with non-English characters in siteurl. The link becomes something like this:

<a href='...%d1%82%d0%b5%d1%81%d1%82/wp-admin/edit-comments.php?p=4' title='0 pending' class='post-com-count'><span class='comment-count'>%s</span></a>

[scribu]

Ah, right, because sprintf() thinks those encoded chars are placeholders. Makes sense now.

[nacin]

$pending_phrase also needs to be escaped.

[nacin]

The URL should also be escaped.

Regardless, patch leaves an issue, due to the nature of comments_number(). If comments > 1, then comments_number() will do an str_replace() on the %. This borks the URL.

[nacin]

Proper function here is get_comments_number(), wrapped with number_format_i18n(). Anything else is extra we don't need or leverage, due to the arguments we pass comments_number().

[nacin]

New patch.

[nacin]

I was wondering why we were using comments_number() like this.

In 3.0, we had code that looked like the red in r15504 -- basically, massive strings going into comments_number(). We also didn't wrap the link in admin_url(), thus we had no extra percentages to screw things up. sprintf() plus admin_url(), two pretty innocuous changes, caused the regression.

The smarter structuring in r15504 in turn revealed that get_comments_number() is a better approach.

[ryan]

Looks good, tests good. Ship it.

[nacin]

(In [17475]) Use get_comments_number() in comments_bubble() method. Removes chance of sprintf arguments error due to percent encoding in URLs and kills unnecessary translations. Escape translations into attributes. esc_url on admin_url. fixes #16611 for trunk.

[nacin]

(In [17476]) Use get_comments_number() in comments_bubble() method. Removes chance of sprintf arguments error due to percent encoding in URLs and kills unnecessary translations. Escape translations into attributes. esc_url on admin_url. fixes #16611 for the 3.1 branch.

[SergeyBiryukov]

16611.2.diff​ doesn't fix the initial issue. printf() throws the same error message, as there are still encoded chars in the URL.

Added new patch.

[duck_]

Do we even need the printf? Why not just string concatenation?

[SergeyBiryukov]

Added patch with string concatenation.

[nacin]

I was going to cycle back around and just string concatenate this, but decided to leave it for 3.2. Didn't see the extra bug in my testing.

Looks good. Commit and ship.

[nacin]

(In [17481]) Avoid printf entirely. props SergeyBiryukov, fixes #16611 for trunk.

[nacin]

(In [17482]) Avoid printf entirely. props SergeyBiryukov, fixes #16611 for 3.1.