BuddyPress: Uncaught ValueErrror from printf-style messages.

[dd32]

Bug Report

Similar to #1508, the BuddyPress connector (And it appears others, by reading code) are still affected by that..

Uncaught ValueError: Unknown format specifier "D" in wp-content/plugins/stream/classes/class-log.php:127
Stack trace:
#0 wp-content/plugins/stream/classes/class-log.php(127): vsprintf('Unmarked activi...', Array)
#1 [internal function]: WP_Stream\Log->log('buddypress', 'Unmarked activi...', Array, 9894139, 'forums', 'unspammed', 0)
#2 wp-content/plugins/stream/classes/class-connector.php(178): call_user_func_array(Array, Array)
#3 wp-content/plugins/stream/connectors/class-connector-buddypress.php(685): WP_Stream\Connector->log('Unmarked activi...', Array, 123456, 'forums', 'unspammed')

Expected Behavior

The $message passed to Log::log() should be a printf-ready string, instead it's often containing prepared data.

Actual Behavior

stream/connectors/class-connector-buddypress.php

Lines 678 to 689 in 393ab0c

	$this->log(
	sprintf(
	/* translators: %s: an activity title (e.g. "Update") */
	__( 'Unmarked activity "%s" as spam', 'stream' ),
	wp_strip_all_tags( $activity->action )
	),
	array(
	'id' => $activity->id,
	'item_id' => $activity->item_id,
	'type' => $activity->type,
	'author' => $activity->user_id,
	),

In the above case, vsprintf( 'Unmarked activity "something 5% Do something"', [ 'id' => ?, 'item_id' => ??, ... ] ) is called, which is obviously wrong.

It appears that a number of connectors are calling log() and passing context to the vsprintf args key, instead of args that should be interpolated into the message.

Here's an example of a correct usage:

stream/connectors/class-connector-users.php

Lines 155 to 169 in 393ab0c

	/* translators: %1$s: a user display name, %2$s: a user role (e.g. "Jane Doe", "subscriber") */
	$message = _x(
	'New user account created for %1$s (%2$s)',
	'1: User display name, 2: User role',
	'stream'
	);
	$user_to_log = $current_user->ID;
	}
	$this->log(
	$message,
	array(
	'display_name' => ( $registered_user->display_name ) ? $registered_user->display_name : $registered_user->user_login,
	'roles' => implode( ', ', $this->get_role_labels( $user_id ) ),
	),

Here's another example of an incorrect usage (That happens to pass, as the first key of the $args happens to be the one wanted)

stream/connectors/class-connector-comments.php

Lines 238 to 245 in 393ab0c

	$this->log(
	/* translators: %s: a username (e.g. "administrator") */
	__( 'Comment flooding by %s detected and prevented', 'stream' ),
	compact( 'user_name', 'user_id', 'time_lastcomment', 'time_newcomment' ),
	null,
	'comments',
	'flood'
	);

This is a little more prevalent in the plugin than I expected, but appears to mostly not be an issue as it's rare for the $message to contain a % character.

[dd32]

Personally speaking, I'd drop printf support in the Log::log( $message ) arg and require all connectors/uses perform the sprintf at that point in time, converting the $args into a "Message context" parameter instead. That however prevents the filters from being able to filter the saved message by altering the passed data 🤷