Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
23 changes: 23 additions & 0 deletions tests/AbstractTestCase.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

namespace Psr\Log\Util\Tests;
Copy link
Member

Choose a reason for hiding this comment

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

I totally missed the namespace issue, maybe from #1.

This namespace is different from before, and it's below autoload-dev, so it's not available to end users like this.

We should move this under src and under autoload. Also, we should check if we want to also copy the old namespace, but that would conflict with psr/log ^1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. I would contend, however, that it still belongs under tests with all the namespaces like this, except it should move to autoload. After all, these are actual tests that we are running on CI.

About conflict with 1.x, I don't think two packages that share a namespace is necessarily a problem, and it is definitely not prohibited: I do it sometimes, when it is semantically advantageous. However, I put them under Util because they are not part of the spec (AFAIK), so it's just utility implementations which as I mentioned before are IMHO not relevant to the specification, just like any other implementation.

Copy link

Choose a reason for hiding this comment

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

Util packages have generally used the Fig\$Lib namespace. The Psr namespace is for the specs themselves, period. We should follow that here.

Including an alias file for the old names is fine if people want to make it a bit easier to transition. (That is, a file that contains a bunch of class_alias statements that people can require if desired.)

Copy link
Contributor Author

@XedinUnknown XedinUnknown Jun 22, 2021

Choose a reason for hiding this comment

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

Arguably, this would be a massive BC break, though. Is there consensus for applying this here? As far as I understand, the purpose of this PR is to port everything with max compatibility - with both the original spec and the current PHP code. Although, I don't really understand why: people who use psr/log 1.x will have this in that package, and people who use 2.x will need e.g. PHP 8.0 and fixing many BC breaks anyway. So I would suggest maybe putting this stuff back in the original namespace, and then break BC everywhere necessary at once.

Copy link
Member

Choose a reason for hiding this comment

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

With the class aliases, the BC would still be there, and we would also avoid the conflict that I was talking about, so I think it would be the better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it not be considered a BC-break if the file with aliases has to be included separately and optionally? Perhaps, we could file-autoload it?

By the way, maybe a philosophical question, but an interesting one to me: is declaration of class aliases code that declares symbols, or code that has an effect?

Copy link
Member

Choose a reason for hiding this comment

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

IMHO it's symbols; but I would file-autoload it and wrap all class_aliases in if (! class_exists(...)) so to avoid conflicts.


if (class_exists('PHPUnit\Framework\TestCase')) {
Copy link

Choose a reason for hiding this comment

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

Please use TestCase::class et al any time you reference a class, not a string. That's valid since PHP 5.5 and makes the code much more maintainable and readable.

Copy link
Contributor Author

@XedinUnknown XedinUnknown Jun 22, 2021

Choose a reason for hiding this comment

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

I used a class-string because this upcoming release targets PHP 5.4 and 5.3 as well, and this naturally raises syntax errors. If we change this, we are betraying the spec. Which I would be OK with if you still think this is a good idea, as long as we acknowledge that and do it knowingly 😃

Copy link

Choose a reason for hiding this comment

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

Test classes are not part of the spec, so they can be whatever the heck we want. 😄

The stats from Packagist show that PHP < 5.5 installs are barely 2%, and PHP 8 is 7 times as widely used: https://packagist.org/packages/psr/log/php-stats

So I officially do not give a damn about PHP versions 5.4 and lower.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test classes are not part of the spec, so they can be whatever the heck we want.

Not part of the spec, but the spec says there's support for 5.3 and 5.4, and strictly speaking we must build on those versions too in order to ascertain that this package actually complies with the spec.

So I officially do not give a damn about PHP versions 5.4 and lower.

Fair enough. Does there need to be some consensus on this, or should I just do it? Sorry, I don't really know how things work between you 😅

Copy link
Member

Choose a reason for hiding this comment

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

Since this is aimed to replace the stripped classes from psr/log, I would support 5.3 but drop it immediately after with another release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I'd also do, if my understanding of the goals of this first release is correct ☝️

class AbstractTestCase extends \PHPUnit\Framework\TestCase
{
public function setExpectedException($exceptionName, $exceptionMessage = '', $exceptionCode = null)
{
if (method_exists($this, 'expectException')) {
$this->expectException($exceptionName, $exceptionMessage, $exceptionCode);
return;
}

parent::setExpectedException($exceptionName, $exceptionMessage, $exceptionCode);
}
}
} elseif (class_exists('PHPUnit_Framework_TestCase')) {
// phpcs:ignore PSR1.Classes.ClassDeclaration.MultipleClasses
class AbstractTestCase extends \PHPUnit_Framework_TestCase
{
}
}
151 changes: 151 additions & 0 deletions tests/LoggerInterfaceTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
<?php

namespace Psr\Log\Util\Tests;

use DateTimeZone;
use Psr\Log\LoggerInterface;
use Psr\Log\LogLevel;

/**
* Provides a base test class for ensuring compliance with the LoggerInterface.
*
* Implementors can extend the class and implement abstract methods to run this
* as part of their test suite.
*/
abstract class LoggerInterfaceTest extends AbstractTestCase
{
/**
* @return LoggerInterface
*/
abstract public function getLogger();

/**
* This must return the log messages in order.
*
* The simple formatting of the messages is: "<LOG LEVEL> <MESSAGE>".
*
* Example ->error('Foo') would yield "error Foo".
*
* @return string[]
*/
abstract public function getLogs();

public function testImplements()
{
$this->assertInstanceOf('Psr\Log\LoggerInterface', $this->getLogger());
}

/**
* @dataProvider provideLevelsAndMessages
*/
public function testLogsAtAllLevels($level, $message)
{
$logger = $this->getLogger();
$logger->{$level}($message, array('user' => 'Bob'));
$logger->log($level, $message, array('user' => 'Bob'));

$expected = array(
"$level message of level $level with context: Bob",
"$level message of level $level with context: Bob",
);
$this->assertEquals($expected, $this->getLogs());
}

public function provideLevelsAndMessages()
{
return array(
LogLevel::EMERGENCY => array(LogLevel::EMERGENCY, 'message of level emergency with context: {user}'),
LogLevel::ALERT => array(LogLevel::ALERT, 'message of level alert with context: {user}'),
LogLevel::CRITICAL => array(LogLevel::CRITICAL, 'message of level critical with context: {user}'),
LogLevel::ERROR => array(LogLevel::ERROR, 'message of level error with context: {user}'),
LogLevel::WARNING => array(LogLevel::WARNING, 'message of level warning with context: {user}'),
LogLevel::NOTICE => array(LogLevel::NOTICE, 'message of level notice with context: {user}'),
LogLevel::INFO => array(LogLevel::INFO, 'message of level info with context: {user}'),
LogLevel::DEBUG => array(LogLevel::DEBUG, 'message of level debug with context: {user}'),
);
}

public function testThrowsOnInvalidLevel()
{
$logger = $this->getLogger();

$this->setExpectedException('Psr\Log\InvalidArgumentException');
$logger->log('invalid level', 'Foo');
}

public function testContextReplacement()
{
$logger = $this->getLogger();
$logger->info('{Message {nothing} {user} {foo.bar} a}', array('user' => 'Bob', 'foo.bar' => 'Bar'));

$expected = array('info {Message {nothing} Bob Bar a}');
$this->assertEquals($expected, $this->getLogs());
}

public function testObjectCastToString()
{
$string = uniqid('DUMMY');
$dummy = $this->createStringable($string);
$dummy->expects($this->once())
->method('__toString');

$this->getLogger()->warning($dummy);

$expected = array("warning $string");
$this->assertEquals($expected, $this->getLogs());
}

public function testContextCanContainAnything()
{
$closed = fopen('php://memory', 'r');
fclose($closed);

$context = array(
'bool' => true,
'null' => null,
'string' => 'Foo',
'int' => 0,
'float' => 0.5,
'nested' => array('with object' => $this->createStringable()),
'object' => new \DateTime('now', new DateTimeZone('Europe/London')),
'resource' => fopen('php://memory', 'r'),
'closed' => $closed,
);

$this->getLogger()->warning('Crazy context data', $context);

$expected = array('warning Crazy context data');
$this->assertEquals($expected, $this->getLogs());
}

public function testContextExceptionKeyCanBeExceptionOrOtherValues()
{
$logger = $this->getLogger();
$logger->warning('Random message', array('exception' => 'oops'));
$logger->critical('Uncaught Exception!', array('exception' => new \LogicException('Fail')));

$expected = array(
'warning Random message',
'critical Uncaught Exception!'
);
$this->assertEquals($expected, $this->getLogs());
}

/**
* Creates a mock of a `Stringable`.
*
* @param string $string The string that must be represented by the stringable.
* @return \PHPUnit_Framework_MockObject_MockObject A mock of an object that has a `__toString()` method.
*/
protected function createStringable($string = '')
{
$mock = $this->getMockBuilder('Stringable')
->setMethods(array('__toString'))
->getMock();

$mock->method('__toString')
->will($this->returnValue($string));

return $mock;
}
}
194 changes: 194 additions & 0 deletions tests/Stub/TestLogger.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
<?php

namespace Psr\Log\Util\Tests\Stub;

use Psr\Log\AbstractLogger;
use Psr\Log\InvalidArgumentException;
use ReflectionClass;

/**
* Used for testing purposes.
*
* It records all records and gives you access to them for verification.
*
* @method bool hasEmergency($record)
* @method bool hasAlert($record)
* @method bool hasCritical($record)
* @method bool hasError($record)
* @method bool hasWarning($record)
* @method bool hasNotice($record)
* @method bool hasInfo($record)
* @method bool hasDebug($record)
*
* @method bool hasEmergencyRecords()
* @method bool hasAlertRecords()
* @method bool hasCriticalRecords()
* @method bool hasErrorRecords()
* @method bool hasWarningRecords()
* @method bool hasNoticeRecords()
* @method bool hasInfoRecords()
* @method bool hasDebugRecords()
*
* @method bool hasEmergencyThatContains($message)
* @method bool hasAlertThatContains($message)
* @method bool hasCriticalThatContains($message)
* @method bool hasErrorThatContains($message)
* @method bool hasWarningThatContains($message)
* @method bool hasNoticeThatContains($message)
* @method bool hasInfoThatContains($message)
* @method bool hasDebugThatContains($message)
*
* @method bool hasEmergencyThatMatches($message)
* @method bool hasAlertThatMatches($message)
* @method bool hasCriticalThatMatches($message)
* @method bool hasErrorThatMatches($message)
* @method bool hasWarningThatMatches($message)
* @method bool hasNoticeThatMatches($message)
* @method bool hasInfoThatMatches($message)
* @method bool hasDebugThatMatches($message)
*
* @method bool hasEmergencyThatPasses($message)
* @method bool hasAlertThatPasses($message)
* @method bool hasCriticalThatPasses($message)
* @method bool hasErrorThatPasses($message)
* @method bool hasWarningThatPasses($message)
* @method bool hasNoticeThatPasses($message)
* @method bool hasInfoThatPasses($message)
* @method bool hasDebugThatPasses($message)
*/
class TestLogger extends AbstractLogger
Copy link

Choose a reason for hiding this comment

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

I'll be honest, this whole class seems heavily over-engineered to me. I'm working on the logger in TYPO3 right now, and using this as a test anon class:

$this->trackingLogger = new class() implements LoggerInterface {
    use LoggerTrait;
    public array $records = [];
    public function log($level, $message, array $context = []): void
    {
        $this->records[] = [
            'level' => $level,
            'message' => $message,
            'context' => $context
        ];
    }
};

Having a pre-made class to help with that is well and good and I'm all for it, but I don't see why that needs 200 lines and dozens of virtual methods (especially when virtual methods are generally evil).

Copy link
Member

Choose a reason for hiding this comment

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

IMO the PHP 5.3 requirement here is at play too, so we can ditch this all with a 2.0 release immediately afterward.

Copy link

Choose a reason for hiding this comment

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

Wouldn't just a records(string $level): array method be ample, and then I can test that however I want myself with the standard PHPUnit tools?

Copy link

Choose a reason for hiding this comment

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

Even in ancient PHP versions, log() like above and a records(string $level): array (even untyped) that returns an empty array if nothing was recorded would be sufficient. Give or take context values, I suppose, but this whole thing can be vastly simplified.

Copy link
Contributor Author

@XedinUnknown XedinUnknown Jun 22, 2021

Choose a reason for hiding this comment

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

I completely agree with this class being over-engineered. In fact, I have had the thought quite a few times that some methods seem to be extra - and the only method e.g. getRecords() that is really necessary was missing - I added it now. Besides that, there was missing functionality: this test logger wasn't actually interpolating context values into the messages before recording them, and I added that in this PR.

Also, I'm not sure how or whether this was being used in the old package, because AFAIK the only test that is actually running now was added by me, and I can't really think about a different application for this class other than for being used in a test to prove that the test tests what it's suppose to test (sorry for the tautology).

Yet still, my humble opinion is that this change (the possible simplification of this class) should happen in the next BC-breaking release.

{
/**
* @var array
*/
public $records = array();

public $recordsByLevel = array();

/**
* @inheritdoc
*/
public function log($level, $message, array $context = array())
{
if (!in_array($level, $this->getLogLevels(), true)) {
throw new InvalidArgumentException(sprintf('Log level "%1$s" is not valid', $level));
}
Comment on lines +73 to +75
Copy link
Member

Choose a reason for hiding this comment

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

This check may be too strict, since the spec says:

Calling this method with a level not defined by this specification MUST throw a Psr\Log\InvalidArgumentException if the implementation does not know about the level. Users SHOULD NOT use a custom level without knowing for sure the current implementation supports it.

Maybe we can add a constructor argument to toggle the strictness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, although it would make it more cumbersome, it sounds like it could be a reasonable decision. But IMHO, since this is a test logger (not an abstract one), it doesn't really matter - because this is our implementation that is only used for testing (or is it used/usable for something else?).

Copy link
Member

Choose a reason for hiding this comment

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

My thought would be to make it hot-swappable with the previous implementation, so that it could be easily used in place of the one that we're stripping from psr/log.

Having the behavior toggable would make it just more powerful and adherent to the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, if you think this is the way. If the log level is "invalid", should it silently log it, or silently fail?


$record = array(
'level' => $level,
'message' => $this->formatMessage($message, $level, $context),
'context' => $context,
);

$this->recordsByLevel[$record['level']][] = $record;
$this->records[] = $record;
}

public function interpolateContext($message, array $context)
{
return preg_replace_callback('!\{([^\}\s]*)\}!', function ($matches) use ($context) {
$key = isset($matches[1]) ? $matches[1] : null;
if (array_key_exists($key, $context)) {
return $context[$key];
}

return $matches[0];
}, $message);
}

public function formatMessage($message, $level, array $context)
{
$message = $this->interpolateContext($message, $context);
$message = "$level $message";

return $message;
}

public function getLogLevels()
{
$reflection = new ReflectionClass('Psr\Log\LogLevel');
$constants = $reflection->getConstants();

return $constants;
}

public function getRecords()
{
return $this->records;
}
Comment on lines +87 to +118
Copy link
Member

Choose a reason for hiding this comment

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

All this methods are new and should be documented as new features.

Copy link
Contributor Author

@XedinUnknown XedinUnknown Jun 22, 2021

Choose a reason for hiding this comment

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

If I were to do it, I would probably add that information to the changelog. Is this what you mean?

On the other hand, like I suggested in another comment, this looks like a totally internal class (I think you mentioned yourself that it is not part of the API that can break etc), merely a stub, and so perhaps it doesn't matter.


public function hasRecords($level)
{
return isset($this->recordsByLevel[$level]);
}

public function hasRecord($record, $level)
{
if (is_string($record)) {
$record = array('message' => $record);
}
return $this->hasRecordThatPasses(function ($rec) use ($record) {
if ($rec['message'] !== $record['message']) {
return false;
}
if (isset($record['context']) && $rec['context'] !== $record['context']) {
return false;
}
return true;
}, $level);
}

public function hasRecordThatContains($message, $level)
{
return $this->hasRecordThatPasses(function ($rec) use ($message) {
return strpos($rec['message'], $message) !== false;
}, $level);
}

public function hasRecordThatMatches($regex, $level)
{
return $this->hasRecordThatPasses(function ($rec) use ($regex) {
return preg_match($regex, $rec['message']) > 0;
}, $level);
}

/**
* Determines whether the logger has logged matching records of the specified level.
*
* @param callable(array{level: \Psr\Log\LogLevel::*, message: string, context: array<mixed>}, int): bool $predicate
* The function used to evaluate whether a record matches.
* @param \Psr\Log\LogLevel::* $level The level of the record
* @return bool True if a matching record has been logged; false otherwise.
*/
public function hasRecordThatPasses($predicate, $level)
{
if (!isset($this->recordsByLevel[$level])) {
return false;
}
foreach ($this->recordsByLevel[$level] as $i => $rec) {
if (call_user_func($predicate, $rec, $i)) {
return true;
}
}
return false;
}

public function __call($method, $args)
{
if (preg_match('/(.*)(Debug|Info|Notice|Warning|Error|Critical|Alert|Emergency)(.*)/', $method, $matches) > 0) {
$genericMethod = $matches[1] . ('Records' !== $matches[3] ? 'Record' : '') . $matches[3];
$level = strtolower($matches[2]);
if (method_exists($this, $genericMethod)) {
$args[] = $level;
return call_user_func_array(array($this, $genericMethod), $args);
}
}
throw new \BadMethodCallException('Call to undefined method ' . get_class($this) . '::' . $method . '()');
}

public function reset()
{
$this->records = array();
$this->recordsByLevel = array();
}
}
Loading