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

Fix warnings in case of incorrect data #482

Closed
wants to merge 5 commits into from
Closed

Fix warnings in case of incorrect data #482

wants to merge 5 commits into from

Conversation

vkhramtsov
Copy link

If somebody pass incorrect input array to format, php will raise several notices, because no any checks performed before access to array indexes.
Please review.

@Seldaek
Copy link
Owner

Seldaek commented Dec 28, 2014

I think either the code needs to handle missing keys like the LogstashFormatter does or you just should not call it with invalid records, but rejecting messages silently is not good.

@vkhramtsov
Copy link
Author

In our code we perform checks that value returned from formatter is instance of Message.
Anyway, I can throw an Exception in case of invalid data.
About silently ignore invalid data: your code actually does it in \Monolog\Formatter\NormalizerFormatter::normalize. From my point of view, code should not raise warnings or notices in any case, even if called with incorrect parameters.

@Seldaek
Copy link
Owner

Seldaek commented Dec 29, 2014

I don't see where the normalizer ignores data. Anyway I would be ok to throwing an exception if some essential keys are needed (or if they are not essential, then do a isset check and go through with a default value or null when they're missing), but the current PR is not ok for me.

@@ -98,4 +102,21 @@ public function format(array $record)

return $message;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a DocBlock here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@vkhramtsov
Copy link
Author

Updated, please review.

@vkhramtsov
Copy link
Author

@Seldaek, what do you think about this pr?

@Seldaek Seldaek closed this in 5cd99de Mar 1, 2015
@Seldaek
Copy link
Owner

Seldaek commented Mar 1, 2015

I fixed it in another way so it mostly works except if essential data is missing then you get an exception.

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.

None yet

3 participants