[BP Legacy Template pack] A printf function is written incorrectly related to "mentions" count in src/bp-templates/bp-legacy/buddypress/activity/index.php

[emaralive]

This bug was flagged on Slack by @venutius (see ​conversation) which was originally reported by @mike80222 as a Support forum post (see ​topic).

Although the offending printf() function is contained within a template file for the BuddyPress Legacy Template, the reported bug can be duplicated by other methods.

The aforementioned function is located on line 164 of ​src/bp-templates/bp-legacy/buddypress/activity/index.php (master branch) which is represented by the following code snippet

<?php
printf( esc_html( _nx( '%s new', '%s new', bp_get_total_mention_count_for_user( bp_loggedin_user_id() ), 'Number of new activity mentions', 'buddypress' ), esc_html( bp_get_total_mention_count_for_user( bp_loggedin_user_id() ) ) ) );

PHP 7.4.33 casts the following error:

Warning: printf(): Too few arguments in...

PHP 8.0.30 casts the following error:

Fatal error: Uncaught ArgumentCountError: 2 arguments are required, 1 given in...

Tracing things back, it looks like the esc_html() function was initially introduced in 12.0.0-beta1 and is a quandary as to why this wasn't caught from a syntactical standpoint. Perhaps the question is: How would one detect such an error? I ask because when utilizing a site with:

WP: 6.6.1
BP: 14.0.0
Theme: Twentyeleven
Template pack: Legacy
PHP: 7.4.33 & 8.0.30

I'm not picking up any error log entries, things appear to be normal (well, as normal can be) and I get an actual "mentions" count. The attached screenshot is indicative of such, i.e., "mentions" count.

Neither here nor there, I'm not sure why the _nx() is needed, the word "new"" is both singular and plural because "news", if it were used, would be a synonym for "information" and not the plural for "new". And, in reality, there is nothing really to contextualize since there should only be a count (numeric indicator) without the word "new".

@espellcaste I stated what I stated, so, here is the ticket, so that you can/may perform your magic.

[emaralive]

Example of "mentions" count

PHP Fatal error:  Uncaught ArgumentCountError: 2 arguments are required, 1 given in /var/www/bp-single/wp-content/plugins/buddypress/src/bp-templates/bp-legacy/buddypress/activity/index.php:164
Stack trace:

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/9221

[espellcaste]

@emaralive Thanks! Added pr with a fix, in case you want to test it. I confirmed locally using PHP 8.1 and PHP 8.2.

[emaralive]

2nd screenshot

[emaralive]

@espellcaste I figured out in order for me to see the failing of the original template file, I have to override the template within a child theme and the effects of line 164 can be seen for both PHP 7.4.33 and 8.0.30. IOW, if I don't override the template file, then I see what is shown in the 1st screenshot.

So, now the question is: Why must the template file be overridden to trigger the error?

I added another screenshot, what if you made the "Mentions" tab look as what is indicated (IOW, drop the word "new": Would that be a problem?

Additionally, I believe other improvements could be made, e.g., change printf() to sprintf() and then use esc_html_e() as the outer function and in the case where HTML is involved use wp_kses()as the outer function along with echo: Would this be a problem? I ask because it seems as though the escaping process isn't effective as written; meaning have another look at the entire file to make sure we are escaping properly.

BTW, I tested your patch and it does not trigger an error.

[espellcaste]

@emaralive Strange. I didn't need to override the template and could see it right away. ¯\_(ツ)_/¯

I only selected the legacy template pack and Twenty Twenty-Four theme.

[espellcaste]

I added another screenshot, what if you made the "Mentions" tab look as what is indicated (IOW, drop the word "new": Would that be a problem?

I think that's a good idea. o/

@imath what do you think?

printf() to sprintf()

There is nothing to say this is better. More preference. Using wp_kses here is more expensive to check. I think esc_html is fine for that usecase.

[DJPaul]

The line was introduced a long time ago. It was probably almost certainly a typo. r6773 rings a bell so perhaps it was an attempt to be mindful of that.

[emaralive]

Replying to espellcaste:

@emaralive Strange. I didn't need to override the template and could see it right away. ¯\_(ツ)_/¯

I only selected the legacy template pack and Twenty Twenty-Four theme.

@espellcaste yeah, that is really odd/strange and a bit disconcerting for a variety of reasons. I have 2 local sites running with the "Legacy" template, however, both are running on the same server (linux debian based under WSL2):

• Twenty Eleven theme (indicated by the 1st screenshot)
• Twenty Eleven with child theme (indicated by the 2nd screenshot)

Changing themes (including to Twenty Twenty-Four) does not trigger any malfunction except when I override the index.php in a child theme and then the malfunction can be observed for PHP 7.4.33 and 8.0.30. I have another server that is linux centos based that I've not tried to see if a site on it would behave differently.

However, when I utilize either "wp shell" or "psy shell" (WP CLI) and run the offending line (original 164) this triggers the errors (PHP 7.4.33 & 8.0.30), which I did first without having to load up a website via a web browser, thus confirming there was an issue. Otherwise, I would have discovered this bug much earlier, like during the testing of various BP 12.0.0 releases; or, at least, I would think this would have been the case.

If this is behavior for some (what I'm experiencing [linux debian based situation]) and not for others, does that explain a reason why this has not been detected since the release of BP 12.0.0? Or, it has occurred and nobody bothered to report it or what?. Perhaps, the sites that are using the legacy template haven't upgraded to greater than or equal to BP 12. Or, they have upgraded and no user has used @mention. I know of a site that uses the legacy template and I've used @mention and nobody has reported an issue for this situation nor have I experienced this issue on this site, I believe the site admin has not upgraded BP past 12 and could still be at the original BP version when the site was created (around 2019).

Moving along, I just tested on my other server (linux centos based) and I get the error on one of the sites after I switched the template pack to legacy, after posting @mention and refreshing the sitewide activity page. I then:

• Rolled back to 11.4.2 - issue does not exist
• Rolled forward to 12.0.0-beta1 - issue exists

I don't know, but, perhaps, this speaks volumes regarding the testing protocols; meaning we don't seem to have any that are sufficient enough and they aren't documented, as of yet.

Last but not least, I have another server scenario which is linux ubuntu based, I just need to reboot the server into ubuntu.

[emaralive]

@espellcaste OK, let's just move this along and commit your PR as is. As previously mentioned your patch works as expected and I ran the tests from my linux centos based server and as a template override from the linux debian based server with same results for patch and without patch. The error messages and new screenshots are from a site on the linux centos based server.

Without patch the error cast for PHP 8.0.30 (see screenshot php80_without_patch.png):

Fatal error: Uncaught ArgumentCountError: 2 arguments are required, 1 given in /wp-content/plugins/buddypress/bp-templates/bp-legacy/buddypress/activity/index.php:164

Without patch the error cast for PHP 7.4.33 (see screenshot php74_without_patch.png):

Warning: printf(): Too few arguments in /wp-content/plugins/buddypress/bp-templates/bp-legacy/buddypress/activity/index.php on line 164

Screenshot screenshot-win10-me-2024.07.29-12_29_59 (1).png (1st screenshot posted) represents what it would look like with patch (just ignore the comments).

[emaralive]

php80_without_patch.png

[emaralive]

php74_without_patch.png

@renatonascalves the code looks good to me but, you may want to get a 2nd opinion.

[espellcaste]

In 14001:

Fix: Corrected argument mismatch in printf() function in the Legacy Template pack.

Addressed an issue where the printf() function in the BuddyPress Legacy Template was causing errors in PHP 7.4.33 and PHP 8.0.30 due to an incorrect number of arguments.

Props venutius, mike80222, imath, emaralive.

See #9221 (trunk)
Closes ​https://github.com/buddypress/buddypress/pull/355

[espellcaste]

In 14002:

Fix: Corrected argument mismatch in printf() function in the Legacy Template pack.

Addressed an issue where the printf() function in the BuddyPress Legacy Template was causing errors in PHP 7.4.33 and PHP 8.0.30 due to an incorrect number of arguments.

Props venutius, mike80222, imath, emaralive.

Fixes #9221 (branch 14.0)