Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[HttpKernel] Let Monolog handle the creation of log folder for improved readonly containers handling #58564

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

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

shyim
Copy link
Contributor

@shyim shyim commented Oct 14, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

When running Symfony in a Container in readonly mode. Symfony checks if the var/log is writeable and it's not because of readonly FS.

I can set env APP_LOG_DIR=/tmp/log, to fix the issue. But this folder will be never used as my monolog only outputs to stdout.

Therefore I would suggest removing this completely and let monolog create the folder, only when the user configuration wants this

@nicolas-grekas
Copy link
Member

How will the app behave of monolog is not used? And how does monolog fail if configurated to write there?

@shyim
Copy link
Contributor Author

shyim commented Oct 17, 2024

How will the app behave of monolog is not used?

No var/log is created

And how does monolog fail if configurated to write there?

When logging it throws a exception:

In StreamHandler.php line 140:
                                                                                                                                                                 
  The stream or file "<project>/var/log/prod-2024-10-17.log" could not be opened in append mode: Failed to open stream: Permission denied  
  The exception occurred while attempting to log: Error thrown while running command "asdasd". Message: "Command "asdasd" is not defined."                       
  Context: {"exception":{},"command":"asdasd","message":"Command \"asdasd\" is not defined."}          

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 23, 2024

I think we periodically wonder about this, and the conclusion has also been: a writeable var/log is a feature of the kernel, it doesn't relate to monolog directly.
I think it's fine for apps with non-conventional needs (readonly fs) to have to do a few more tweaks.
APP_LOG_DIR=/tmp/log is the way to go to me.

@shyim
Copy link
Contributor Author

shyim commented Oct 23, 2024

Maybe deprecate it for Symfony 8.0, remove this method, and rely only on configuration for path /var/log?

getLogDir is just passed as a parameter to the DI container.

Sure I can set APP_LOG_DIR to tmpdir, it feels hacky. Symfony Cloud (platform.sh) seems to have a hack that this folder is always writeable, it would also improve the official Symfony product

@fabpot
Copy link
Member

fabpot commented Oct 23, 2024

As mentioned by @nicolas-grekas, we've had this conversation a few times now. I think more and more containers are read-only nowadays. I'm fine with this change, but probably not as a bug fix for 5.4. I would be ok to merge it in 7.2 though.

@shyim shyim changed the base branch from 5.4 to 7.2 October 23, 2024 15:27
@shyim
Copy link
Contributor Author

shyim commented Oct 23, 2024

updated the target branch

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 7.2 Oct 25, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@fabpot
Copy link
Member

fabpot commented Jan 5, 2025

Thank you @shyim.

@fabpot fabpot merged commit e33dbfa into symfony:7.3 Jan 5, 2025
8 of 12 checks passed
@fabpot fabpot mentioned this pull request May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.