WP_Community_Events Disable Event Logs

[Howdy_McGee]

Every time a user logs into WordPress, WP_Community_Events triggers maybe_log_events_response() writing to the error log that it received data, even whenever it's valid data:

[14-Jun-2017 11:16:07 UTC] WP_Community_Events::maybe_log_events_response: Valid response received. Details: {$JSON_DATA}

Where $JSON_DATA is the returned information. It's valid, there's no real issue I would just like the ability to turn off these logs or disable it from writing whenever the response is successful. It's just additional unnecessary noise in the debug log.

[iandunn]

I wonder if we should actually just turn the logs off by default.

We originally added them because the info is very helpful in troubleshooting reports of problems (like incorrect geolocation/geocoding results), but we haven't been getting many, so it's probably not worth cluttering logs for the 99%.

In that case, it'd probably be best to remove the WP_DEBUG_LOG condition and just use the filter, so that, when we do ask people for the logs, it's only 1 step to enable them, rather than 2.

[voldemortensen]

@iandunn good point. 41217.2.diff simplifies that conditional.

[SergeyBiryukov]

The filter needs to be documented as per the ​PHP Documentation Standards.

[charlestonsw]

My 2-cents -

You're proposing adding another filter which is a lot of extra overhead. You're adding data to the memory stack and forcing WP Core to pull through the filter list and run callbacks.

IMO it would be a better solution overall to force the Community Events developers to add a define to wp-config.php if they DO want to execute this logging. This seems like a far simpler solution and one that is specific to the class at hand versus hijacking the default DEBUG_LOG behavior for a corner case.

		if ( ! WP_COMMUNITY_EVENTS_LOG ) {
			return;
		}

[iandunn]

Replying to charlestonsw:

You're proposing adding another filter which is a lot of extra overhead. You're adding data to the memory stack and forcing WP Core to pull through the filter list and run callbacks.

I think that's a good point. Even a tiny thing like this adds up when combined with all the other filters.

if ( ! WP_COMMUNITY_EVENTS_LOG ) {

I think it'd need to be if ( ! defined( 'WP_COMMUNITY_EVENTS_LOG' ) || ! WP_COMMUNITY_EVENTS_LOG ), since PHP evaluates undefined constants as a string with the name as the value (i.e., 'WP_COMMUNITY_EVENTS_LOG'), which then evaluates to true.

A name likeWP_DEBUG_LOG_EVENTS might be a bit better, since it'd be consistent with the related constants.

[iandunn]

Replying to voldemortensen:

41217.2.diff simplifies that conditional.

This is minor and maybe just personal preference, but I think the early return is a bit more readable than putting the logic inside the if.

It avoids an unnecessary layer of indentation, and separates the function's pre-condition from the function's core logic (both visually and mentally), so that they're smaller and separate chunks. I think that makes it easier and quicker to mentally process them when skimming the function, at least for me.

It also keeps the diff much smaller and targeted to the exact change that's desired.

[iandunn]

Replying to iandunn:

I think it'd need to be if ( ! defined( 'WP_COMMUNITY_EVENTS_LOG' ) || ! WP_COMMUNITY_EVENTS_LOG )

Actually, it'd probably be better to just explicitly define a default false value, so that the constant gets documented in wp_initial_constants().

[iandunn]

Minor style tweaks, switch to a constant

[iandunn]

Based on some ​feedback in Slack, it sounds like filters are Core's preferred approach. There's a tiny bit of overhead, but they're more flexible. The overhead is less than it used to be, though, thanks to r38571.

[dd32]

IMHO: The logging features/functionality of the widget should be pulled entirely, as it's not useful to the majority of users (and as far as I can tell, it's usefulness is decreasing day-by-day, as conditions requiring it should be decreasing).

If debugging data is required, it feels like this would be better suited to a plugin which can expose the data in the WordPress interface in a useful manner, reachable by the non-technical users and those without log access on their host (quite a lot of people have no idea how to access those).
If hooks are needed to facilitate a debugging plugin, add them.

[iandunn]

Remove the logs entirely, see comment:11

[iandunn]

Example mu-plugin to log API requests

[iandunn]

Actually, now that you mention that, I agree that that'd be even better, thanks :)

A plugin can get the needed data via http_api_debug. An example of that is log-community-events-requests.php​.

41217.4.diff​ removes the logging entirely.

[iandunn]

Deprecate the method instead of removing it

[iandunn]

In 41316:

Dashboard: Discontinue nonessential logging of Events API requests.

These log entries are only useful when troubleshooting unexpected results from the API, which is not common. The vast majority of users are better served by not having their logs cluttered with noise.

For the rare situations where troubleshooting is necessary, it can be achieved by a plugin (see #41217 for an example).

Props Howdy_McGee, dd32.
Fixes #41217.

[charlestonsw]

if ( ! WP_COMMUNITY_EVENTS_LOG ) {

I think it'd need to be if ( ! defined( 'WP_COMMUNITY_EVENTS_LOG' ) || ! WP_COMMUNITY_EVENTS_LOG ), since PHP evaluates undefined constants as a string with the name as the value (i.e., 'WP_COMMUNITY_EVENTS_LOG'), which then evaluates to true.

Yes, that is true... I was giving a short non-functional concept. :)

Glad to see the removal of the excess logging and/or filter overhead is likely to get baked into core soon.

[iandunn]

Implode the URL for easier copy/paste, keep event titles in response body, add error mock