8000 [Monolog Bridge][DX] Add a Monolog activation strategy for ignoring specific HTTP codes by simshaun · Pull Request #23707 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Monolog Bridge][DX] Add a Monolog activation strategy for ignoring specific HTTP codes #23707

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

Merged
merged 2 commits into from
Apr 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bridge\Monolog\Handler\FingersCrossed;

use Monolog\Handler\FingersCrossed\ErrorLevelActivationStrategy;
use Symfony\Component\HttpKernel\Exception\HttpException;
use Symfony\Component\HttpFoundation\RequestStack;

/**
* Activation strategy that ignores certain HTTP codes.
*
* @author Shaun Simmons <shaun@envysphere.com>
*/
class HttpCodeActivationStrategy extends ErrorLevelActivationStrategy
Copy link

Choose a reason for hiding this comment

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

It should not extend ErrorLevelActivationStrategy but rather implement ActivationStrategyInterface, since this strategy doesn't deal with the error levels.

Copy link
Contributor

Choose a reason for hiding this comment

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

Plus, this class could be final IMO.

Copy link
Contributor Author
@simshaun simshaun Feb 1, 2018

Choose a reason for hiding this comment

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

@pimolo The FingersCrossedHandler leaves it up to the activation strategy object to handle the action level. It defaults to the ErrorLevelActivationStrategy only when it isn't explicitly given an object implementing ActivationStrategyInterface. Because an HttpCodeActivationStrategy object is given to the FingersCrossedHandler when the excluded_http_codes configuration exists, the HttpCodeActivationStrategy becomes responsible for handling the action level, just like the existing NotFoundActivationStrategy does. So, that's why it's extending the ErrorLevelActivationStrategy class.

{
private $exclusions;
private $requestStack;

/**
* @param array $exclusions each exclusion must have a "code" and "urls" keys
*/
public function __construct(RequestStack $requestStack, array $exclusions, $actionLevel)
{
foreach ($exclusions as $exclusion) {
if (!array_key_exists('code', $exclusion)) {
throw new \LogicException(sprintf('An exclusion must have a "code" key'));
}
if (!array_key_exists('urls', $exclusion)) {
throw new \LogicException(sprintf('An exclusion must have a "urls" key'));
}
}

parent::__construct($actionLevel);

$this->requestStack = $requestStack;
$this->exclusions = $exclusions;
}

public function isHandlerActivated(array $record)
{
$isActivated = parent::isHandlerActivated($record);

if (
$isActivated
&& isset($record['context']['exception'])
&& $record['context']['exception'] instanceof HttpException
&& ($request = $this->requestStack->getMasterRequest())
Copy link
Member

Choose a reason for hiding this comment

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

$request is not used

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't being used before. It is now with the addition of URL blacklisting.

) {
foreach ($this->exclusions as $exclusion) {
if ($record['context']['exception']->getStatusCode() !== $exclusion['code']) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be useful to ensure that the code key exists, and throw otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

Done in the constructor

continue;
}

$urlBlacklist = null;
if (count($exclusion['urls'])) {
return !preg_match('{('.implode('|', $exclusion['urls']).')}i', $request->getPathInfo());
}

return false;
}
}

return $isActivated;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bridge\Monolog\Tests\Handler\FingersCrossed;

use Monolog\Logger;
use PHPUnit\Framework\TestCase;
use Symfony\Bridge\Monolog\Handler\FingersCrossed\HttpCodeActivationStrategy;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpKernel\Exception\HttpException;

class HttpCodeActivationStrategyTest extends TestCase
{
/**
* @expectedException \LogicException
*/
public function testExclusionsWithoutCode()
{
new HttpCodeActivationStrategy(new RequestStack(), array(array('urls' => array())), Logger::WARNING);
}

/**
* @expectedException \LogicException
*/
public function testExclusionsWithoutUrls()
{
new HttpCodeActivationStrategy(new RequestStack(), array(array('code' => 404)), Logger::WARNING);
}

/**
* @dataProvider isActivatedProvider
*/
public function testIsActivated($url, $record, $expected)
{
$requestStack = new RequestStack();
$requestStack->push(Request::create($url));

$strategy = new HttpCodeActivationStrategy(
$requestStack,
array(
array('code' => 403, 'urls' => array()),
array('code' => 404, 'urls' => array()),
array('code' => 405, 'urls' => array()),
array('code' => 400, 'urls' => array('^/400/a', '^/400/b')),
),
Logger::WARNING
);

$this->assertEquals($expected, $strategy->isHandlerActivated($record));
}

public function isActivatedProvider()
{
return array(
array('/test', array('level' => Logger::ERROR), true),
array('/400', array('level' => Logger::ERROR, 'context' => $this->getContextException(400)), true),
array('/400/a', array('level' => Logger::ERROR, 'context' => $this->getContextException(400)), false),
array('/400/b', array('level' => Logger::ERROR, 'context' => $this->getContextException(400)), false),
array('/400/c', array('level' => Logger::ERROR, 'context' => $this->getContextException(400)), true),
array('/401', array('level' => Logger::ERROR, 'context' => $this->getContextException(401)), true),
array('/403', array('level' => Logger::ERROR, 'context' => $this->getContextException(403)), false),
array('/404', array('level' => Logger::ERROR, 'context' => $this->getContextException(404)), false),
array('/405', array('level' => Logger::ERROR, 'context' => $this->getContextException(405)), false),
array('/500', array('level' => Logger::ERROR, 'context' => $this->getContextException(500)), true),
);
}

protected function getContextException($code)
{
return array('exception' => new HttpException($code));
}
}
0