Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Steps to reproduce:
Add basic_auth to standard.info.yml, try to install with another language than EN.
This happens:
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "current_user", path: "plugin.cache_clearer -> plugin.manager.menu.local_action -> access_manager -> url_generator -> route_processor_manager -> route_processor_csrf -> csrf_token -> current_user -> authentication -> authentication.basic_auth -> user.auth". in Symfony\Component\DependencyInjection\Container->get() (line 283 of .../core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php).
The problem is that in the constructor of basic auth auth provider and UserAuth, we call $entity_manager->getStorage(), which looks up definitions if this happens during a plugin cache clear after installing a module, then it does @Translation('User') or similar, which goes into the LocaleLookup cache, which needs the user roles. Boom.
Proposed resolution
The workaround is fairly easy: Do not call $entity_manager->getStorage() in the constructor of those services, instead keep the entity manager around.
This makes sense anyway (those services are invoked on every request and shouldn't create stuff they might not need) but it's very fragile...
Remaining tasks
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | do-not-call-storage-in-construct-2241461-9-test-only.patch | 791 bytes | Berdir |
| #9 | do-not-call-storage-in-construct-2241461-9.patch | 5.11 KB | Berdir |
| #9 | do-not-call-storage-in-construct-2241461-9-interdiff.txt | 1.53 KB | Berdir |
Comments
Comment #1
BerdirSetting to critical because enabling basic_auth on a non-english site will make it kaputt.
Comment #2
BerdirWill need some tests I guess...
Comment #4
juampynr CreditAttribution: juampynr commentedPossibly unrelated: after applying the patch and while running the installer I saw this error once all modules got installed:
Comment #5
juampynr CreditAttribution: juampynr commentedIgnore my above comment. My local db was not cleared up before reinstalling.
Now looking into those failing tests.
Comment #6
BerdirI think I just forgot to update one of the $this->userStorage in those classes.
Comment #7
juampynr CreditAttribution: juampynr commentedThere was a missing line that needed to be changed. Let's see if all tests pass now.
Comment #9
BerdirOk, here we go, extended InstallerTranslationTest to enable basic_auth through the UI, so that we can do it in a non-english context, this fails as expected.
Also fixed the unit test to no longer assume how often getStorage() is called, because it is called as often as we need it, which is ok, we have an optimized static cache for entity storages.
Comment #10
BerdirComment #12
dawehner+1
Comment #13
juampynr CreditAttribution: juampynr commentedLooks good to me too.
Comment #14
catchFeels like the circular reference here is really because translate entity info when it's requested as opposed to rendered. Most of the time we don't care what the translations are, certainly not in this case. That would require #1213510: A modern t() to fix though.
Comment #15
catchCommitted/pushed to 8.x, thanks!
Opened #2244447: Translation of low-level info/annotations leads to circular dependencies because I agree with berdir this seems extremely fragile still.
Comment #16
catchI haven't successfully pushed this yet. Git prompted me for a password locally, and drupalcode.org is a 500. Will try to remember to push later, but re-opening so we don't discover a missing commit in two months...
Comment #18
Berdir#17 says this is now pushed.