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

Add a helper function that makes it easier to log from anywhere #26582

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

ChristophWurst
Copy link
Member

Our DI is able to inject a logger implementation to any server and app
class if they want one. However, sometimes DI isn't applicable or hard
to add. In those cases we typically fell back to the service locator
pattern where we acquired a logger from the server via a global
variable.

There were some issues with that

  • \OC is a private class, apps are not supposed to use it
  • \OC::$server is a global variable, a well known anti-pattern
  • \OC::$server->get(...) uses the service locator anti-pattern
  • \OC::$server->get(...) may throw
  • \OC::$server->get(LoggerInterface::class) is not scoped to an app

With this patch I'm proposing a new helper function \OCP\Log\logger
that can be used to acquire a logger more easily. This function is meant
to be public API and therefore apps may use it and there is an optional
parameter to specifiy the app ID.
The function hides all the ugly details about the global variable and
the potentially thrown exceptions from the user. Therefore it's
guaranteed that you always get a logger instance. In the worst case you
get a noop, though those occasions should be rare.

The intended usage is either

use function OCP\Log\Logger;

logger('mail')->warning('user did something bad');

or quick and dirty (e.g. for some debugging)

\OCP\Log\logger('mail')->warning('the user did something bad');

This idea is based on a recent discussion with @nickvergessen and @rullzer. Let me know what you think.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Seems like a step in the right direction!

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 16, 2021
@rullzer
Copy link
Member

rullzer commented Apr 16, 2021

But CI says no

@kesselb
Copy link
Contributor

kesselb commented Apr 20, 2021

Fix the path in composer.json. I don't know why composer dump-autoload produces a different result.

composer -V
Composer version 1.10.21 2021-04-01 09:16:34

@nickvergessen
Copy link
Member

Use composer 2

@kesselb
Copy link
Contributor

kesselb commented Apr 20, 2021

Use composer 2

Good point. I think we need composer 1 for some horde repositories. Will install it as composer2 and run dump-autoload again.

@kesselb kesselb force-pushed the enhancement/logger-function branch from 92cb7d4 to 29b66f5 Compare April 20, 2021 21:04
@ChristophWurst
Copy link
Member Author

I gave this another thought over the last few days based on some input by @miaulalala. I would still say a free function is preferable over a static method on a class. Putting some bad memories from the old static APIs aside, I think the class approach will make it too tempting to introduce state via static variables, and therefore pull in all the trouble of impure "functions". The current free function is pure-ish, the result does depend on some side effects but at least the function is guaranteed to not change any state. IMO this should be the preferred way.

@skjnldsv
Copy link
Member

Please rebase :)

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 18, 2021
@ChristophWurst ChristophWurst force-pushed the enhancement/logger-function branch 2 times, most recently from d068d47 to 9c80fe5 Compare February 23, 2022 08:17
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 23, 2022
@ChristophWurst ChristophWurst added this to the Nextcloud 24 milestone Feb 23, 2022
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
This was referenced Apr 7, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 17, 2022
@skjnldsv
Copy link
Member

/rebase

Our DI is able to inject a logger implementation to any server and app
class if they want one. However, sometimes DI isn't applicable or hard
to add. In those cases we typically fell back to the *service locator*
pattern where we acquired a logger from the server via a global
variable.

There were some issues with that
* `\OC` is a private class, apps are not supposed to use it
* `\OC::$server` is a global variable, a well known anti-pattern
* `\OC::$server->get(...)` uses the service locator anti-pattern
* `\OC::$server->get(...)` may throw
* `\OC::$server->get(LoggerInterface::class)` is not scoped to an app

With this patch I'm proposing a new helper function ``\OCP\Log\logger``
that can be used to acquire a logger more easily. This function is meant
to be public API and therefore apps may use it and there is an optional
parameter to specifiy the app ID.
The function hides all the ugly details about the global variable and
the potentially thrown exceptions from the user. Therefore it's
guaranteed that you always get a logger instance. In the worst case you
get a noop, though those occasions should be rare.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@nickvergessen
Copy link
Member

Any changes resulting from \OCP\Server::get() being created? Should we keep the method approach here or maybe create Server::log()?

@ChristophWurst
Copy link
Member Author

#26582 (comment) I stand by that.

@ChristophWurst
Copy link
Member Author

@juliushaertl @PVince81 @icewind1991 any preference on your side?

@juliusknorr
Copy link
Member

I'd be all in favour for the current approach of that PR to have it somehow independent API wise from the server container.

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Jun 1, 2022
@ChristophWurst ChristophWurst merged commit 92d6461 into master Jun 7, 2022
@ChristophWurst ChristophWurst deleted the enhancement/logger-function branch June 7, 2022 09:22
@ChristophWurst ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
Development

Successfully merging this pull request may close these issues.

7 participants