search

phalcon / cphalcon

High performance, full-stack PHP framework delivered as a C extension.
https://phalcon.io
BSD 3-Clause "New" or "Revised" License
10.79k stars 1.96k forks source link

[BUG]: Logger variable %type% not in uppercase #15375

Closed wurst-hans closed 3 years ago

wurst-hans commented 3 years ago

Using Phalcon v4.1.0 I've set following format for logger [%date%] [%type%] %message% but this results in:

[2021-04-05T09:52:56+02:00] [warning] Use of undefined constant a - assumed 'a' (this will throw an Error in a future version of PHP) in /code/src/Bootstrap/Application.php on line 21
[2021-04-05T10:12:57+02:00] [notice] Undefined variable: y in /code/Bootstrap/Application.php on line 20

I expect, that type of message is written in uppercase.

kowach commented 3 years ago

Same here. I see that Logger::getLevels() are hard coded lowercase. But why?

niden commented 3 years ago

@kowach @wurst-hans Can you guys have a look at this PR?

https://github.com/phalcon/cphalcon/pull/15401

Maybe I am setting the test wrong but that is how I understood it from your description. Any pointers welcome.

Thanks!

wurst-hans commented 3 years ago

Sorry, don't understand much of testing, but as I see in your PR, the test expects $expected = '[info] info message ' . $unique; but shouldn't it be $expected = '[INFO] info message ' . $unique;. The log level has to be in upper case according to documentation

niden commented 3 years ago

I was trying to replicate the error you posted

[warning] Use of undefined constant a - assumed 'a'

I thought that the undefined constant... was coming from the type conversion.

If it is the uppercase that is the issue, we can certainly change the docs (easiest) or change the code. I don't mind doing either.

@Jeckerson thoughts?

Jeckerson commented 3 years ago

@niden I think we should change to uppercase in the code, as in docs it always was uppercase, but never was noticed.

@kowach Because it is not intended to be changed. However, the method is protected and you can easily extend it and put naming as you wish.

wurst-hans commented 3 years ago

This isn't a showstopper for me and isn't worth it to overwrite protected getLevels() method. I just wondered about one more thing, silently changed. Because in Phalcon v3.4 the log level was printed uppercase to log (see code). And from most other projects, the log level is used to be printed uppercase, too. So I prefer a change of code, but no hurry.

niden commented 3 years ago

In case anyone is wondering. I remember why this was changed:

https://github.com/php-fig/log/blob/master/Psr/Log/LogLevel.php#L10

PSR has everything lowercase.

We will change it to upper to keep with 3.4. Maybe put an option in to have them up or down or something

wurst-hans commented 3 years ago

Thanks for clarification. No problem if PSR requires it to be lowercase. But then it makes no sense do switch back to uppercase, doesn't it?

niden commented 3 years ago

PSR does not "require" it I think so long as you can identify what the level is - uppercase or lowercase. I checked Monolog and they print out their levels in uppercase.

I will change it to the way 3.4 was (uppercase). Also since getLevels is protected, it can be overridden if someone wants it to be lowercase

I will have the PR ready shortly

niden commented 3 years ago

This has been resolved for v5