-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
{ | ||
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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you are wrong, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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']) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} 9CCB td> | ||
|
||
/** | ||
* @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)); | ||
} | ||
} |
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.
It should not extend
ErrorLevelActivationStrategy
but rather implementActivationStrategyInterface
, since this strategy doesn't deal with the error levels.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.
Plus, this class could be
final
IMO.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.
@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.