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

fix: change session start log level #9221

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

element-code
Copy link
Contributor

@element-code element-code commented Oct 11, 2024

Description
The session class clutters out-of-the-box configured logs with debug information about its driver initialization on startup, using a INFO classification.
This is especially problematic in authenticated webapps as every request will write at least one info message.
The message itself has a debug purpose and therefore should be classified as DEBUG

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@element-code element-code changed the title Change session start log level refactor: change session start log level Oct 11, 2024
@michalsn
Copy link
Member

I agree, that using info may pollute the logs unnecessarily. I wouldn’t be against treating this as a bug.

Still, we will need a changelog entry.

Side note... Items on the issue description checklist should only be marked if they are accurate - thanks!

@kenjis
Copy link
Member

kenjis commented Oct 12, 2024

I don't think this is a bug, but I would not be against to change to DEBUG.
I think it is better that this PR goes to 4.6 branch.

@element-code element-code changed the title refactor: change session start log level fix: change session start log level Oct 12, 2024
@element-code
Copy link
Contributor Author

Which file I need to edit for the changelog entry @michalsn?
I think this could be treated as a bug, since it only relates to application debugging and shouldn't be breaking in any production environment, therefore.

@michalsn
Copy link
Member

@element-code This one: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.5.6.rst

Signed-off-by: Wolf Wortmann <[email protected]>
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Thank you.

In general, we don't enter the PR number in the changelog.

user_guide_src/source/changelogs/v4.5.6.rst Outdated Show resolved Hide resolved
Co-authored-by: Michal Sniatala <[email protected]>
@element-code
Copy link
Contributor Author

Alright, should I squash or do you squash to merge?

@michalsn
Copy link
Member

We can squash on merge - we just have to wait for one more approval.

Copy link
Contributor

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

This is a great improvement, it was always bothering me!🤓

@michalsn michalsn merged commit c8b9ea4 into codeigniter4:develop Oct 15, 2024
41 of 42 checks passed
@michalsn
Copy link
Member

@element-code Thank you!

@mbnl
Copy link

mbnl commented Oct 15, 2024

This was a very frustrating situation for me, so I'm glad it got resolved. Also, are there no new features or updates being made to CI4? Has its development been stopped

@michalsn
Copy link
Member

Also, are there no new features or updates being made to CI4? Has its development been stopped

@mbnl What do you mean?

The latest release was a month ago: https://github.com/codeigniter4/CodeIgniter4/releases/tag/v4.5.5

The new features go to the 4.6 branch: https://github.com/codeigniter4/CodeIgniter4/tree/4.6, which is ahead of the develop by more than 300 commits.

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants