-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Timestamp log filter #6058
Timestamp log filter #6058
Conversation
|
||
if ($this->value instanceof DateTime) { | ||
return version_compare((string) $timestamp, (string) $this->value->getTimestamp(), $this->operator); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need else
, just return directly because already return early in previous if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do understand your point, but I prefer to use explicit else
in these kind of situations, in order to "highlight" that two different strategies/directions. On the other hand, in case of raw boolean return statements, I use that "no else" approach, with that last, fallback return statement at the very end of a method declaration.
But if you insist, I'll alter this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already did a lot of work at the pass for removing else
because already return early, see #3286
$this->dateFormatChar = $dateFormatChar; | ||
} | ||
|
||
$this->operator = ($operator) ?: '<='; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the operator be validated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I frankly haven't seen any validation on $operator in Zend\Log\Filter\Priority, which is also based on the version_compare() function. Do you think that I should at least add some type check (is string)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikolaposa what are the allowed operator here? The PHP docs mention The possible operators are: <, lt, <=, le, >, gt, >=, ge, ==, =, eq, !=, <>, ne respectively.
.
The fact that the priority filter doesn't have strict checks doesn't mean that we can't apply them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, no problem, I'll put some validation there.
$value = iterator_to_array($value); | ||
} | ||
if (is_array($value)) { | ||
extract($value, EXTR_IF_EXISTS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use extract; test for values in the array instead, and set the locally declared variables based on what you find in the array.
…Testing string versions of comparison operators.
Why is this build failing? I suppose that the problem is in some other commit, because test failure points to "ZendTest\Cache\Storage\Adapter\FilesystemTest::testClearExpired". What should be done in such situations, in order to make my commit "green"? Should I rebase with master and then force push? |
@nikolaposa it's a race condition that happens quite often in the builds, sadly :( |
Do I need to take any action on this issue? btw Can this situation be prevented somehow? Maybe with more frequent master rebasing? |
@nikolaposa the bug has not yet been solved, so rebasing won't help anyway. No action required for that failure |
@nikolaposa one thing that is needed here is documentation for the new feature - think you can prepare a PR for that? I've scheduled merging for this once that's done. |
Should I create/submit that pull request to the ZF2 docs project (https://github.com/zendframework/zf2-documentation) or in some other way? |
@nikolaposa that is the correct way :-) |
…to develop Documentation for zendframework/zendframework#6058 (timestamp log filter) Close #1273
…rom a traversable
…`iterator_to_array`
…atetime are discarded
…o read) timestamps
… should be checked too
…xception` symbols
…can be built from a traversable
…Array()` over `iterator_to_array`
…le initialization
…es without a datetime are discarded
…vents without a timestamp
… integer and string timestamps
…dcoded (hard to read) timestamps
…amps (strings) should be checked too
…ntegers and strings
…ng \`@group\` annotation
…filter' into develop Close zendframework/zendframework#6058
Log filter which provides ability to filter log events based on the time when they were triggered.
It can be configured by specifying idate()-compliant format character and desired value or a DateTime instance for direct comparison with the log event's timestamp. Comparison operator must also be supplied in either cases.
Example usage: