inpsyde / Wonolog

Monolog-based logging package for WordPress.
https://inpsyde.github.io/Wonolog/
MIT License
173 stars 17 forks source link

Missing Update Return Type Documentation #42

Closed XedinUnknown closed 4 years ago

XedinUnknown commented 5 years ago

The Problem

ActionListenerInterface#update() documents that the only possible return type is LogDataInterface. However, it is not exactly true: if any other type is returned, there are no negative consequences; instead, no message is logged silently.

Alas, from the interface documentation it is not clear what the handler needs to do in order to avoid logging anything.

Suggested Solution

Add this to the documentation of ActionListenerInterface#update(), so that it becomes e.g. this:

@return LogDataInterface|mixed The data to write, if applicable; anything else otherwise.

Optionally, tighten the alternative return value to a single type, e.g. null.

Alternatively, make LogDataInterface the only valid return value, and amend the check to throw if the data is invalid (or declare the return type in PHP with : LogDataInterface.

gmazzap commented 5 years ago

if any other type is returned, there are no negative consequences; instead, no message is logged silently.

Does not "no message is logged silently" looks like a negative consequence?


@return LogDataInterface|mixed The data to write, if applicable; anything else otherwise

This make me cringe. I consider any occurrence of "mixed" half of a bug waiting to happen, and the word "anything" is not something I would like to ever see written in the doc of a return type. I could be open in documenting:

/**
 * @return LogDataInterface|null Return null means no log happen

And if future version of the library which would support PHP 7.1+ use a ?LogDataInterface return type.


and amend the check to throw if the data is invalid (or declare the return type in PHP with : LogDataInterface.

In all Wonolog code we avoided to throw exception on runtime because who will log exceptions if the logging library raises them?

Chrico commented 4 years ago

In version 2.x we will raise the PHP version and apply our 1.x Coding Standards. So this will be done there.