Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend Additional Field Settings and Added IP Address Support #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cyberwani
Copy link

@cyberwani cyberwani commented Jan 5, 2018

Added support to provide additional parameters for Additional and Extra fields. With this user can provide extra details for each field. So, extra fields can be more flexible.
Added support to store IP address with log.

Added support to provide additional parameters for Additional and Extra fields.
Updated README.md for changes in Additoinal and Extrad Fields Settings.
Added support for ip address.
@cyberwani cyberwani changed the title Extend Additoinal and Extrad Fields Settings Extend Additoinal Field Settings and Added IP Address Support Jan 5, 2018
@BenceSzalai
Copy link
Collaborator

BenceSzalai commented Apr 11, 2020

Hi!

The part about the the additional fields can be done, but line 116 must be

$additionalFields.=",\n`$af` TEXT NULL DEFAULT NULL";

instead of

$additionalFields.=",\n{$af['name']} TEXT NULL DEFAULT NULL";

to avoid incompatibility with exiting implementations. This means if 'type' is provided, it is used, but if not, there is no reason to expect 'name' to be set. If there's no 'type' the value itself is the column name, just like as it is now. (Also note the ` around the column name, which is needed for better compatibility. See here.)

As for the $record['extra'] handling this need to be demonstrated to be compatible with Monolog! I think it is not, as by design when an array is passed as one of the items in 'extra', the array is going to be converted to a string in some way to be logged. Users expect their 'extra' array to be logged as it is, and not to be interpreted in any special ways depending on the contents.
However this should be no issue, as you could simply use $this->additionalFields to declare your 'extra' columns with custom types instead.

More generally, the column types used by monolog-wordpress can become something customized in WordPressHandler as Monolog does not care how the data is stored by Handlers (classes implementing HandlerInterface). However a Handler cannot implement functionality that introduces ambiguity to HandlerInterface. In other words, anyone should be able to replace any Handler with any other Handlers. Such change may affect where and how the extra data is being stored, but should not affect what parts of the extra data are being stored. Trying to interpret certain array keys (like 'name' and 'type') in a special way violates this principle.

Regarding your changes to log the IP addresses, that is certainly not something all users would need or like, and it's not reasonable to force such specific behaviour onto others. For the very same reason our talented friends at Monolog have designed the architecture of the logging stack in a way, that anyone can implement their own 'automatically add some custom data to my log entries' needs. The solution is called Processors. You should implement a custom processor that adds the IP address as an extra field, and push that Processor to your WordPressHandler instance as demonstrated in the example. Actually Monolog\Processor\WebProcessor already logs the client IP, although your method is more advanced, so you may extend that class or just build your own.

PS: it may be better to submit unrelated features as separate PRs, so they can be discussed and approved independently.

@BenceSzalai BenceSzalai changed the title Extend Additoinal Field Settings and Added IP Address Support Extend Additional Field Settings and Added IP Address Support Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants