Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[2.1] Ported FingersCrossed handler from monolog to ZF2 #2271

Closed
wants to merge 3 commits into from

4 participants

@travisbot

This pull request fails (merged fb0461c5 into 89eaf41).

@travisbot

This pull request passes (merged 904e035 into 89eaf41).

@travisbot

This pull request passes (merged 1c54d2d into 89eaf41).

@b-durand

I review at the beginning of the next week.

@b-durand

We have filter classes in ZF2 to determine if the event should be handle by a writer. And, the error level strategy is the same functionality as the PriorityFilter.

I ask myself if we should throw an exception when we set the formatter...

library/Zend/Log/Writer/FingersCrossed.php
((62 lines not shown))
+ * @var ActivationStrategyInterface
+ */
+ protected $activationStrategy;
+
+ /**
+ * Constructor
+ *
+ * @param WriterInterface $writer Wrapped writer
+ * @param ActivationStrategyInterface|int $activationStrategyOrPriority Strategy or log priority which determines buffering of events
+ * @param int $bufferSize Maximum buffer size
+ */
+ public function __construct(WriterInterface $writer, $activationStrategyOrPriority = null, $bufferSize = 0)
+ {
+ $this->writer = $writer;
+
+ if ($activationStrategyOrPriority === null) {
@b-durand
b-durand added a note
null === $activationStrategyOrPriority;

is better than

$activationStrategyOrPriority === null;

Afaik this is just to prevent unwanted assignments if you accidentally omit one of the "=" in comparisons with "=="'.
I personally don't think this is necessary in heavily tested environments as well as in php in general due to the "===" operator.
This should be general decision because there is no rule for that yet. Some components do it this way, some the other way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...riter/FingersCrossed/ErrorLevelActivationStrategy.php
((15 lines not shown))
+ *
+ * @category Zend
+ * @package Zend_Log
+ * @subpackage Writer
+ */
+class ErrorLevelActivationStrategy implements ActivationStrategyInterface
+{
+
+ protected $priority;
+
+ /**
+ * Constructor
+ *
+ * @param int $priority any event priority equals or severe than this will deactivate buffering
+ */
+ public function __construct($priority = Logger::WARN)
@b-durand
b-durand added a note

I don't like the default value: if the parameter is optional, you should use null. And if the value is null, you set a default value. I don't think that we need to define a default priority (we don't have a default priority in our priority filter).

removed the default value

removed the activation strategy ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stefankleff stefankleff Replaced ActivationStrategy with filters.
Removed exception when setting formatter.
d960eb6
@weierophinney

@stefankleff Can you write up:

  • Usage -- give some examples and use cases
  • How this differs from the filter system already present in Zend\Log

From what I can see, our log filters may already do what you're trying to accomplish here, but it's hard to tell, because I'm not 100% clear from the patch how you intend it to be used.

@weierophinney weierophinney reopened this
@stefankleff

The FingersCrossedWriter is based on a simple assumption:
If I log something with a level which is critical, I want to know why this happened.
Usually you log everything with a priority higher than the priority the priority filter allows. This could mess up your logfile or your debug output.
This writer does not write anything until a certain priority is exceeded - But then every logged message is shown, even with a lower priority. This could help to analyse why the critical message was logged without messing up the log output in cases where no critical message was logged.

It's probably better explained at http://packages.python.org/Logbook/api/handlers.html#logbook.FingersCrossedHandler . This is where monolog (which is basically the default symfony logger) has the idea from.

@weierophinney weierophinney commented on the diff
library/Zend/Log/Writer/FingersCrossed.php
((61 lines not shown))
+ *
+ * @param WriterInterface $writer Wrapped writer
+ * @param FilterInterface|int $filterOrPriority Filter or log priority which determines buffering of events
+ * @param int $bufferSize Maximum buffer size
+ */
+ public function __construct(WriterInterface $writer, $filterOrPriority = null, $bufferSize = 0)
+ {
+ $this->writer = $writer;
+
+ if (null === $filterOrPriority) {
+ $this->addFilter(new PriorityFilter(Logger::WARN));
+ } elseif (!$filterOrPriority instanceof FilterInterface) {
+ $this->addFilter(new PriorityFilter($filterOrPriority));
+ } else {
+ $this->addFilter($filterOrPriority);
+ }
@weierophinney Owner

To reduce duplication and make it more readable, I'd recommend:

if (null === $filterOrPriority) {
    $filterOrPriority = new PriorityFilter(Logger::WARN);
} elseif (!$filterOrPriority instanceof FilterInterface) {
    $filterOrPriority = new PriorityFilter($filterOrPriority);
}
$this->addFilter($filterOrPriority);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Log/Writer/FingersCrossed.php
((117 lines not shown))
+ $this->buffer[] = $event;
+
+ if ($this->bufferSize > 0 && count($this->buffer) > $this->bufferSize) {
+ array_shift($this->buffer);
+ }
+
+ if ($this->isActivated($event)) {
+ $this->buffering = false;
+
+ foreach ($this->buffer as $bufferedEvent) {
+ $this->writer->write($bufferedEvent);
+ }
+ }
+ } else {
+ $this->writer->write($event);
+ }
@weierophinney Owner

Move the work into the method body, not the conditional. As an example:

if (!$this->buffering) {
    $this->writer->write($event);
    return;
}
// do rest of work here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney

I'll make the changes as noted in the comments when I merge. Now that I understand it better, this looks like a great addition!

@weierophinney weierophinney referenced this pull request from a commit
@weierophinney weierophinney [#2271] Incorporate feedbac
- Incorporated feedback from comments
- Added @see annotation pointing to python Logbook documentation
- Added writer to WriterPluginManager
576b759
@weierophinney

Merged to the develop branch, to be released with 2.1.

@stefankleff

Thank you for your changes!

@basz basz referenced this pull request from a commit
@weierophinney weierophinney [#2271] Incorporate feedbac
- Incorporated feedback from comments
- Added @see annotation pointing to python Logbook documentation
- Added writer to WriterPluginManager
c2faed9
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney [#2271] Incorporate feedbac
- Incorporated feedback from comments
- Added @see annotation pointing to python Logbook documentation
- Added writer to WriterPluginManager
45b3ebe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 30, 2012
  1. @stefankleff
  2. @stefankleff

    Fixed CS

    stefankleff authored
Commits on Sep 10, 2012
  1. @stefankleff

    Replaced ActivationStrategy with filters.

    stefankleff authored
    Removed exception when setting formatter.
This page is out of date. Refresh to see the latest.
View
165 library/Zend/Log/Writer/FingersCrossed.php
@@ -0,0 +1,165 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Log
+ */
+namespace Zend\Log\Writer;
+
+use Zend\Log\Filter\Priority as PriorityFilter;
+use Zend\Log\Filter\FilterInterface;
+use Zend\Log\Formatter\FormatterInterface;
+use Zend\Log\Exception;
+use Zend\Log\Logger;
+use Zend\Log\Writer\WriterInterface;
+use Zend\Log\Writer\AbstractWriter;
+
+/**
+ * Buffers all events until the strategy determines to flush them.
+ *
+ * @category Zend
+ * @package Zend_Log
+ * @subpackage Writer
+ */
+class FingersCrossed extends AbstractWriter
+{
+
+ /**
+ * The wrapped writer
+ *
+ * @var WriterInterface
+ */
+ protected $writer;
+
+ /**
+ * Flag if buffering is enabled
+ *
+ * @var boolean
+ */
+ protected $buffering = true;
+
+ /**
+ * Oldest entries are removed from the buffer if bufferSize is reached.
+ * 0 is infinte buffer size.
+ *
+ * @var int
+ */
+ protected $bufferSize;
+
+ /**
+ * array of log events
+ *
+ * @var array
+ */
+ protected $buffer = array();
+
+ /**
+ * Constructor
+ *
+ * @param WriterInterface $writer Wrapped writer
+ * @param FilterInterface|int $filterOrPriority Filter or log priority which determines buffering of events
+ * @param int $bufferSize Maximum buffer size
+ */
+ public function __construct(WriterInterface $writer, $filterOrPriority = null, $bufferSize = 0)
+ {
+ $this->writer = $writer;
+
+ if (null === $filterOrPriority) {
+ $this->addFilter(new PriorityFilter(Logger::WARN));
+ } elseif (!$filterOrPriority instanceof FilterInterface) {
+ $this->addFilter(new PriorityFilter($filterOrPriority));
+ } else {
+ $this->addFilter($filterOrPriority);
+ }
@weierophinney Owner

To reduce duplication and make it more readable, I'd recommend:

if (null === $filterOrPriority) {
    $filterOrPriority = new PriorityFilter(Logger::WARN);
} elseif (!$filterOrPriority instanceof FilterInterface) {
    $filterOrPriority = new PriorityFilter($filterOrPriority);
}
$this->addFilter($filterOrPriority);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ $this->bufferSize = $bufferSize;
+ }
+
+ /**
+ * Log a message to this writer.
+ *
+ * @param array $event log data event
+ * @return void
+ */
+ public function write(array $event)
+ {
+ $this->doWrite($event);
+ }
+
+ /**
+ * Check if buffered data should be flushed
+ *
+ * @param array $event event data
+ * @return boolean true if buffered data should be flushed
+ */
+ protected function isActivated(array $event)
+ {
+ foreach ($this->filters as $filter) {
+ if (!$filter->filter($event)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Write message to buffer or delegate event data to the wrapped writer
+ *
+ * @param array $event event data
+ * @return void
+ */
+ protected function doWrite(array $event)
+ {
+ if ($this->buffering) {
+ $this->buffer[] = $event;
+
+ if ($this->bufferSize > 0 && count($this->buffer) > $this->bufferSize) {
+ array_shift($this->buffer);
+ }
+
+ if ($this->isActivated($event)) {
+ $this->buffering = false;
+
+ foreach ($this->buffer as $bufferedEvent) {
+ $this->writer->write($bufferedEvent);
+ }
+ }
+ } else {
+ $this->writer->write($event);
+ }
@weierophinney Owner

Move the work into the method body, not the conditional. As an example:

if (!$this->buffering) {
    $this->writer->write($event);
    return;
}
// do rest of work here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+
+ /**
+ * Resets the state of the handler.
+ * Stops forwarding records to the wrapped writer
+ */
+ public function reset()
+ {
+ $this->buffering = true;
+ }
+
+ /**
+ * Stub in accordance to parent method signature.
+ * Fomatters must be set on the wrapped writer.
+ *
+ * @param Formatter $formatter
+ */
+ public function setFormatter(FormatterInterface $formatter)
+ {
+ return $this->writer;
+ }
+
+ /**
+ * Record shutdown
+ *
+ * @return void
+ */
+ public function shutdown()
+ {
+ $this->writer->shutdown();
+ $this->buffer = null;
+ }
+}
View
57 tests/ZendTest/Log/Writer/FingersCrossedTest.php
@@ -0,0 +1,57 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+*
+* @link http://github.com/zendframework/zf2 for the canonical source repository
+* @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+* @license http://framework.zend.com/license/new-bsd New BSD License
+* @package Zend_Log
+*/
+
+namespace ZendTest\Log\Writer;
+
+use Zend\Log\Writer\FingersCrossed as FingersCrossedWriter;
+use Zend\Log\Writer\Mock as MockWriter;
+use Zend\Log\Logger;
+
+/**
+ * @category Zend
+ * @package Zend_Log
+ * @subpackage UnitTests
+ * @group Zend_Log
+ */
+class FingersCrossedTest extends \PHPUnit_Framework_TestCase
+{
+ public function testBuffering()
+ {
+ $wrappedWriter = new MockWriter();
+ $writer = new FingersCrossedWriter($wrappedWriter, 2);
+
+ $writer->write(array('priority' => 3, 'message' => 'foo'));
+
+ $this->assertSame(count($wrappedWriter->events), 0);
+ }
+
+ public function testFlushing()
+ {
+ $wrappedWriter = new MockWriter();
+ $writer = new FingersCrossedWriter($wrappedWriter, 2);
+
+ $writer->write(array('priority' => 3, 'message' => 'foo'));
+ $writer->write(array('priority' => 1, 'message' => 'bar'));
+
+ $this->assertSame(count($wrappedWriter->events), 2);
+ }
+
+ public function testAfterFlushing()
+ {
+ $wrappedWriter = new MockWriter();
+ $writer = new FingersCrossedWriter($wrappedWriter, 2);
+
+ $writer->write(array('priority' => 3, 'message' => 'foo'));
+ $writer->write(array('priority' => 1, 'message' => 'bar'));
+ $writer->write(array('priority' => 3, 'message' => 'bar'));
+
+ $this->assertSame(count($wrappedWriter->events), 3);
+ }
+}
Something went wrong with that request. Please try again.