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

Log to error_log in fatal case #36316

Merged
merged 1 commit into from
May 23, 2023
Merged

Log to error_log in fatal case #36316

merged 1 commit into from
May 23, 2023

Conversation

R0Wi
Copy link
Member

@R0Wi R0Wi commented Jan 23, 2023

Summary

Write a log message via error_log in case we can't regularly write to our logfile. This improves troubleshooting.

Use case

Example:
We forgot to install the php-apcu module but configured it in config.php. This could happen for example when upgrading the PHP version or when not properly reading the manual.

Before:
When trying to access NC, we're presented with a generic error message.

Internal Server Error

The server encountered an internal error and was unable to complete your request.
Please contact the server administrator if this error reappears multiple times, please include the technical details below in your report.
More details can be found in the server log.

Neither nextcloud.log nor /var/log/apache2/error.log (or something similar) is written. As an admin we absolutely don't know what's going on (Btw: I only solved this by debugging via XDebug).

After:
There is a log entry in /var/log/apache2/error.log telling us:

[Mon Jan 23 17:57:01.559577 2023] [php:notice] [pid 189438] [client ::1:48076] Error when trying to log exception: Memcache \\OC\\Memcache\\APCu not available for local cache 
#0 /var/www/html/nextcloud/lib/private/Server.php(745): OC\\Memcache\\Factory->__construct()
#1 /var/www/html/nextcloud/lib/private/AppFramework/Utility/SimpleContainer.php(162): OC\\Server->OC\\{closure}()
#2 /var/www/html/nextcloud/3rdparty/pimple/pimple/src/Pimple/Container.php(122): OC\\AppFramework\\Utility\\SimpleContainer->OC\\AppFramework\\Utility\\{closure}()
#3 /var/www/html/nextcloud/lib/private/AppFramework/Utility/SimpleContainer.php(129): Pimple\\Container->offsetGet()
...

This is not formatted nicely but it tells the admin what's the problem and why the NC server instance is down.

Checklist

@szaimen szaimen added this to the Nextcloud 26 milestone Jan 23, 2023
@szaimen szaimen added the 3. to review Waiting for reviews label Jan 23, 2023
@szaimen szaimen requested review from a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team January 23, 2023 17:33
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@psyray
Copy link

psyray commented Mar 3, 2023

Thanks for this PR it helps me to identify the problem
It was APCu and memcached PHP extensions missing for me

I don't understand why there is a try..catch without log

@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@R0Wi R0Wi requested review from szaimen and skjnldsv April 15, 2023 17:22
@R0Wi
Copy link
Member Author

R0Wi commented Apr 17, 2023

@nickvergessen I think the CI/CD errors are not related to these changes, right? So could we merge this?

@nickvergessen
Copy link
Member

I think the CI/CD errors are not related to these changes, right?

I would recommend to rebase, to proof it.

So could we merge this?

No, missing 2 approvals

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Fine by me then but didnt test

@R0Wi
Copy link
Member Author

R0Wi commented Apr 18, 2023

The only test currently failing is postgres10-php8.0 :

There was 1 failure:
--
115 |  
116 | 1) Test\User\DatabaseTest::testSearch
117 | Failed asserting that actual size 3 matches expected size 2.
118 |  
119 | /drone/src/tests/lib/User/Backend.php:114
120 | /drone/src/tests/lib/User/DatabaseTest.php:125

Would assume this is not related to the current change?

This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@R0Wi
Copy link
Member Author

R0Wi commented May 23, 2023

Any chance to get this minor change merged, please?

@nickvergessen
Copy link
Member

CI fail is known and unrelated.
So rebasing and setting to merge

Signed-off-by: Robin Windey <ro.windey@gmail.com>
@szaimen
Copy link
Contributor

szaimen commented May 23, 2023

CI failure unrelated

@szaimen szaimen merged commit 3a9abb2 into master May 23, 2023
@szaimen szaimen deleted the improve-fatal-logging branch May 23, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants