Skip to content
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

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

Closed
wurst-hans opened this issue Apr 5, 2021 · 10 comments · Fixed by #15401
Closed

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

wurst-hans opened this issue Apr 5, 2021 · 10 comments · Fixed by #15401
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: low Low

Comments

@wurst-hans
Copy link

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.

@wurst-hans wurst-hans added bug A bug report status: unverified Unverified labels Apr 5, 2021
@kowach
Copy link

kowach commented Apr 13, 2021

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

@niden
Copy link
Member

niden commented Apr 14, 2021

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

#15401

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

Thanks!

@wurst-hans
Copy link
Author

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
Copy link
Member

niden commented Apr 15, 2021

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
Copy link
Member

@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
Copy link
Author

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.

@Jeckerson Jeckerson added status: low Low and removed status: unverified Unverified labels Apr 15, 2021
@niden
Copy link
Member

niden commented Apr 16, 2021

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

@niden niden added the 5.0 The issues we want to solve in the 5.0 release label Apr 16, 2021
@wurst-hans
Copy link
Author

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
Copy link
Member

niden commented Apr 16, 2021

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
Copy link
Member

niden commented Apr 19, 2021

This has been resolved for v5

@niden niden closed this as completed Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: low Low
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants