Problem/Motivation

When src/Logger/Raven.php::getClient() is called very early, it creates a circular dependency that can cause several serious side effects. In my case, it completely broke interface language negotiation: when a user changed their language, the content updated but the interface remained stuck in the site's default language (English). Caches must be enabled. The issue does not occur on the 6.x branch because $this->currentUser->hasPermission('send logs to sentry') does not exist there.

Circular reference detected for service "router.route_provider", path: "options_request_listener -> router.route_provider -> cache_tags.invalidator -> plugin.manager.block -> logger.channel.default -> monolog.handler.error_raven_filter -> logger.raven -> router -> router.no_access_checks".

Steps to reproduce

TBD

Issue fork raven-3584527

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bobooon created an issue. See original summary.

bobooon opened merge request !67

bobooon’s picture

bobooon commented
Issue summary: View changes
bobooon’s picture

bobooon commented
Status: Active » Needs review

The MR moves the new "send logs to sentry" permission check from the getClient() method, which executes well before Drupal authentication occurs, to the log() method. This fixes the circular dependency and, ultimately, the interface language negotiation. However, this might not cover situations where log() is called before authentication.

bobooon’s picture

bobooon commented
Issue summary: View changes
bobooon’s picture

bobooon commented
Related issues: +#3541687: Support Sentry structured logs
mfb’s picture

mfb
they or he
commented

The way I implemented it (so far) is that we need to know early on - when initializing Sentry - whether or not logging should be enabled or disabled. But if checking permissions has side effects then perhaps we simply cannot gate logging behind a user permission :/

I wish it was better documented what has side effects but I guess we have to assume that almost anything might, if/when run early enough? In general most people aren't trying to add logic that might run very early in bootstrap so it's somewhat unexplored territory.

mfb’s picture

mfb
they or he
commented

@bobooon One thing I'm confused about re: your fix is that you'd think that we catch such a circular reference via the try/catch statement you are moving? Are you sure the exception is thrown here and not earlier, e.g. when instantiating service dependencies for the logger?

mfb’s picture

mfb
they or he
commented
Status: Needs review » Postponed (maintainer needs more info)

I haven't yet figured out how to reproduce this issue. Can we have some steps to reproduce from a clean install of Drupal core?

mfb opened merge request !68

mfb’s picture

mfb
they or he
commented
Status: Postponed (maintainer needs more info) » Needs review

New MR removes the problematic "send logs to sentry" permission

  • mfb committed 45c7677b on 7.x 
    fix: #3584527 Circular reference detected for service

By: mfb
By:...

mfb closed merge request !67

mfb’s picture

mfb
they or he
commented
Status: Needs review » Fixed

Hopefully resolved? 🤞

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

gabesullice’s picture

gabesullice
Primary language English
commented

@mfb: confirming that when I applied this as a patch to the latest release, the issue was resolved.

You can use this module to reproduce the error on 7.3.9 without this MR/patch.

mfb’s picture

mfb
they or he
commented
Status: Closed (fixed) » Active
Issue tags: +Needs tests

In that case, let's add a test

mfb’s picture

mfb
they or he
commented
Status: Active » Postponed (maintainer needs more info)

Hmm I actually still can't reproduce the problem with that test module installed, and the fix reverted. I just get the Type Error reported, not the circular dependency. I wonder if some other module or configuration is required to reproduce?

mfb’s picture

mfb
they or he
commented

@bobooon @gabesullice please update the issue if you can provide steps to reproduce, otherwise I will have to give up on adding a test