Comment module causes Circular reference error on a REST request

Here is some research we (webflo, dawehner) have done.

The problem is basically the following: The csrf_token service needs the current user, so the authentication starts.
Normally (for all cases in tests) we either use the Cookie provider (which does not load the user). In this case csrf token can get the user
and the request continues.

In the case "basic authentication" usecase we authenticate the user and then load the new authenticated user. This will trigger hook_entity_load,
which will call comment_entity_load() which requires the comment.manager service. That one requires the current user and we have the circular dependency.

While debugging we realized a couple of points:

• comment_entity_load() also takes non content entities / non fieldable entities into account
• comment.manager does not pass in the current user as synchronized service
• comment.manager does basically way too much. It does not use the current user for the loading related parts of the code.
  This is a potential sign that we should better split up the service. The word "manager" does not tell us anything about the actual work done by the class. If we split it up we would have solved the circular dependency

In general @damiankloip wanted to work on making the current user not get fetched automatically but '@current_user' should be just some kind of factory, so we authenticate later. This should also solve the problem described in this issue.

The patch does fixes the first and second point but certainly does not fix the actual root of the issue.

In general @damiankloip wanted to work on making the current user not get fetched automatically but '@current_user' should be just some kind of factory, so we authenticate later. This should also solve the problem described in this issue.

That's similar to my suggestion to only have the authentication manager as a service that's passed around (We can still have Drupal::currentUser()), which would essentially be the same.

Patch looks OK, good catch on content entities, I did ask for a hook_content_entity_load somewhere, but forget to come back for this cleanup

Just tried authencation.patch and went through listed steps and got the error:

Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException: in Drupal\Core\EventSubscriber\AccessSubscriber->onKernelRequestAccessCheck() (line 87 of /home/s77aec018cc5a771/www/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php)

Let's add the comment module to the test as juampy did, if only temporarily, so that we can know when this is actually passing.

To fix that properly the CommentManager::forbiddenMessage() should be moved out manager, probably to CommentViewBuilder

From IRC:
<alexpott> andypost: I'd be tempted to split CommentManager into two - CommentManager that does the higher level stuff with users etc... and CommentFieldManager (or something like that) which does the low level entity management stuff.

Related #2180109: Change the current_user service to a proxy

1. +++ b/core/modules/comment/comment.module
   @@ -854,6 +854,9 @@ function comment_translation_configuration_element_submit($form, &$form_state) {
    function comment_entity_load($entities, $entity_type) {
   +  if (!\Drupal::entityManager()->getDefinition($entity_type)->isSubclassOf('Drupal\Core\Entity\ContentEntityInterface')) {
   +    return;
   

   comment_entity_load() should gone in #148849: Refactor {comment_entity_statistics} into performant Field

2. +++ b/core/modules/comment/comment.services.yml
   @@ -7,7 +7,9 @@ services:
      comment.manager:
   ...
   +    calls:
   +      - [setCurrentUser, ['@current_user=']]
   
   +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -86,15 +84,18 @@ class CommentManager implements CommentManagerInterface {
   +  public function setCurrentUser(AccountInterface $current_user) {
   +    $this->currentUser = $current_user;
   

   Wrong way!
   Manager should not depends on current_user at all

This error occurs as well for a content type 'article' on a clean install without a comment field.

Comment manager should not depend on user.
So patch removes this dependency.

PS: there's issue for split #2188287: Split CommentManager in two

Now with comment module dependency as #12 to test dependecies

Debugged this, actual problem caused by url_generator dependency!
Current user calls user_authenticate() that calls user_load_by_name() that invokes entity system too early, so other services is trying to initialize... the comment manager needs url_generator to up but container detects that this service in progress of creation... and fails.

discussed in IRC with @dawehner, my point is - no code (csrf is not a exceptional) should be executed before auth,
So #2180109: Change the current_user service to a proxy can't help with the issue

Yes, it could have done if we went with one of the earlier patches. But people seem to be against that. That issue already mentions that point, so maybe have a look.

I think this works now #2180109: Change the current_user service to a proxy is in?

This worked for me.

+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -37,13 +37,6 @@ class CommentManager implements CommentManagerInterface {
-  protected $currentUser;

Once you remove variable then you need to remove it from constructor and service definition as well

Actually it is the other way round.

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentViewBuilder.php
   @@ -241,14 +241,14 @@ protected static function buildLinks(CommentInterface $entity, EntityInterface $
   +      if (empty($links) && \Drupal::currentUser()->isAnonymous()) {
            $links['comment-forbidden']['title'] = \Drupal::service('comment.manager')->forbiddenMessage($commented_entity, $entity->getFieldName());
   

   then no reason to check the same in manager

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentViewBuilder.php
   @@ -241,14 +241,14 @@ protected static function buildLinks(CommentInterface $entity, EntityInterface $
   -    if ($container->get('module_handler')->moduleExists('content_translation') && content_translation_translate_access($entity)) {
   +    if (\Drupal::moduleHandler()->moduleExists('content_translation') && content_translation_translate_access($entity)) {
   

   Then remove $container aswell

then no reason to check the same in manager

I kinda dislike to do access checking in the manager ...

Then remove $container aswell

Maybe at the same time we could already inject the module handler.

Fixed the points of andrey

+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -277,39 +277,37 @@ public function getFieldUIPageTitle($commented_entity_type, $field_name) {
-    if ($this->currentUser->isAnonymous()) {

That's why I remove current_user from dependencies of manager in #16, this one is only needed here

Aaaaaaah, here is a patch. Thank you for the pointer.

@juampy
The reason might be probably because we have the account proxy now.

Part of patch included in #2188287: Split CommentManager in two

34: comment-2182055-34.patch queued for re-testing.

All this changes are needed and critically

+++ b/core/modules/comment/comment.module
 function comment_entity_load($entities, $entity_type) {
+  if (!\Drupal::entityManager()->getDefinition($entity_type)->isSubclassOf('Drupal\Core\Entity\ContentEntityInterface')) {
+    return;

The primary purpose of the patch

+++ b/core/modules/comment/comment.module
 function comment_entity_load($entities, $entity_type) {
+  if (!\Drupal::entityManager()->getDefinition($entity_type)->isSubclassOf('Drupal\Core\Entity\ContentEntityInterface')) {
+    return;

It's not clear from the issue summary, nor a quick scan through the comments. What else is getting passed in here? At least needs an inline comment.

Does that clarify it?

agreed, exactly

Oh doh it's a generic entity hook so it runs on everything. I'd forgotten we're still loading comment statistics onto entity objects. We should just rip that out, but not here.

Committed/pushed to 8.x, thanks!