-
Notifications
You must be signed in to change notification settings - Fork 4
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
Port Tests #2
Port Tests #2
Conversation
This begins porting as per php-fig/log#76 (comment)
@Jean85, I see that |
No you can't. As said in this other comment, I envision a first release that would be able to replace the original code, and a later release with bumps toward newer PHP & PHPUnit version. Then, maybe we can do a 2.0 here too, that would require 8. |
- Create a padding to provide compatibility between PHPUnit 4 and 8. - Move `TestLogger` to `Stub`. - Add an actual test that will test the `TestLogger`.
@Jean85, GH Actions is not building here. Perhaps needs to be enabled in the settings. |
Attempt to make compatible with PHP 5.3.
- Format messages. - Validate log level. - Expose entries. - Normalize entries for test. - Fixed date not having a timezone (important if no timezone set on server).
This ensures that we build on all PHP versions, every time.
@Jean85, the tests are passing for me locally on PHP 5.5. Not sure why they are failing here. If you have any ideas, I'd love to hear them! 😃 |
This will allow contributors to detect problems faster and easier, avoiding scenarios where a problem is corrected only to find that there is a different problem that was not visible in CI logs before on e.g. another PHP version. See discussion: php-fig#2 (comment)
This reverts commit 82ce817, according to php-fig#2 (comment). See php-fig#3.
This removes the need for this extra class, giving the test more control. This also re-factors some tests, making them easier to understand.
This was incompatible with PHP 5.3. The new docs far more correctly describe the contract.
In this particular scenario, this is necessary to make the creation of tests easier.
The `setUp()` method signature here was not compatible with later PHPUnit versions due to the `void` typehint in the latter. Debugging output confirmed that a different instance of the logger still gets created for every test after refactoring.
This also addresses php-fig/log#75.
PHPUnit test on at least PHP 5.3 and 5.4 failing because `+00:00` is unrecognized there.
Looks like everything is working. |
Code looks fine for now; I have to compare it with the original code though to understand if we're changing something in their side, to evaluate if the migration from the old code is hard or not. |
Maybe I can help. At least, it's these:
For now, that is all, I think. |
if (!in_array($level, $this->getLogLevels(), true)) { | ||
throw new InvalidArgumentException(sprintf('Log level "%1$s" is not valid', $level)); | ||
} |
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.
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?
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.
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?).
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.
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.
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.
Sure, if you think this is the way. If the log level is "invalid", should it silently log it, or silently fail?
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; | ||
} |
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.
All this methods are new and should be documented as new features.
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.
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.
@@ -0,0 +1,23 @@ | |||
<?php | |||
|
|||
namespace Psr\Log\Util\Tests; |
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 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
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.
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.
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.
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.)
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.
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.
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.
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.
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.
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?
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.
IMHO it's symbols; but I would file
-autoload it and wrap all class_alias
es in if (! class_exists(...))
so to avoid conflicts.
I have in mind the idea to do a |
|
||
namespace Psr\Log\Util\Tests; | ||
|
||
if (class_exists('PHPUnit\Framework\TestCase')) { |
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 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.
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 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 😃
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.
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.
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.
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 😅
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.
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.
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.
That's what I'd also do, if my understanding of the goals of this first release is correct ☝️
* @method bool hasInfoThatPasses($message) | ||
* @method bool hasDebugThatPasses($message) | ||
*/ | ||
class TestLogger extends AbstractLogger |
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'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).
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.
IMO the PHP 5.3 requirement here is at play too, so we can ditch this all with a 2.0 release immediately afterward.
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.
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?
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.
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.
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 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.
@XedinUnknown Any class mentioned in the spec should remain in the main spec package, IMO. It's unfortunate that we did it that way back in 2012, in hindsight it's a mistake, but we're stuck with it for now. Utility classes beyond what's in the spec proper are fine to include here, but those should remain in the main package. |
The spec package updates just passed. What's the status here? |
There were some points that remain unaddressed. I was waiting for feedback, and got distracted. I will go through them again and implement directions that are clear, will comment here about this, and wait for feedback on the remaining points. |
The way I see it, any project upgrading to psr/log 2.0+ will need to support PHP ^7.2 (or at least 7 for the scalar hints, not sure if 7.2 is required for psr/log 2). So I would say this util repo could start off at 1.0 supporting only PHP 7.0 or 7.2, and drop the older PHPUnit versions. That would keep things simpler. Projects stuck maintaining PHP 5 have to remain on psr/log 1.0 anyway so they can keep using the abstract test class from there. |
Can we resume the work here? Jordi's suggestion seems good to me. |
@XedinUnknown Are you going to continue this? |
Hi, this is a friendly reminder. Is there anything blocking progression? |
@XedinUnknown seems unactive. Please feel free to fetch his branch and continue his work following the suggestions from #2 (comment) |
Created an issue to discuss the strategy: #4 |
Just ports the tests from
php-fig/log
.