Entity Field Query throws QueryException when querying all revisions and setting condition on entity reference field where target doesn't support revisions

patch #2 doesn't apply properly on 8.5

patching file core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
Hunk #1 succeeded at 66 (offset -1 lines).
Hunk #2 FAILED at 88.
Hunk #3 FAILED at 157.
Hunk #4 succeeded at 271 with fuzz 2 (offset 65 lines).

Here is how I got this error:

[file_field_paths module is enable]
1- edit user's image field property: admin/config/people/accounts/fields/user.user.user_picture
2-in the "File (Field) Path settings" check "Retroactive update"
3-save the form
4-I got the following error:

Drupal\Core\Entity\Query\QueryException: '' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable() (line 316 of core/lib/Drupal/Core/Entity/Query/Sql/Tables.php). 
Drupal\Core\Entity\Query\Sql\Tables->addField('', 'INNER', NULL) (Line: 44)
Drupal\Core\Entity\Query\Sql\Condition->compile(Object) (Line: 155)
Drupal\Core\Entity\Query\Sql\Query->compile() (Line: 74)
Drupal\Core\Entity\Query\Sql\Query->execute() (Line: 263)
filefield_paths_batch_update(Object) (Line: 238)
filefield_paths_form_submit(Array, Object)
call_user_func_array('filefield_paths_form_submit', Array) (Line: 111)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 51)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 585)
Drupal\Core\Form\FormBuilder->processForm('field_config_edit_form', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm('field_config_edit_form', Object) (Line: 74)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 153)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 656)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

@tedbow, I'm not sure the variable name should be changed from all_revisions to use_revisioin, it seems to me that the only real change needed is the last hunk from the patch :)

Also, as noted in #10, this patch needs a reroll and some test coverage.

Rerolled the patch for Drupal 8.6.x. Unfortunately, it doesn't solve our problem.

We have a problem similar to this with one of our custom modules. The module added a boolean field to the node_field_data table. During cron the value of the field is checked and act on (or not) accordingly. In hindsight it was not a good idea to use node_field_data, but since the custom module does things on every node on the site, it seemed like a good idea at the time. Before Drupal 8.5.x and content moderation, it worked fine.

I'll probably fix the problem by creating a custom table for the boolean field and the node_id. The custom field is not revisionable, which is the cause of my error.

My error message:

ResponseText: The website encountered an unexpected error. Please try again later.Drupal\Core\Entity\Query\QueryException: 'CUSTOM_FIELD_NAME' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable() (line 350 of core/lib/Drupal/Core/Entity/Query/Sql/Tables.php). Drupal\Core\Entity\Query\Sql\Tables->addField('CUSTOM_FIELD_NAME', 'LEFT', NULL) 

@esod, could you please provide more details? How exactly do you add the field? Is it translatable? Is it revisionable? Does it have cardinality? I think that your problem deserves a separate issue.

Also it would be nice if you could address @amateescu's remarks from #11:

@tedbow, I'm not sure the variable name should be changed from all_revisions to use_revisioin, it seems to me that the only real change needed is the last hunk from the patch :)

As I am currently working on issues in this area I've decided to write a test for the issue here, but unfortunately my test isn't failing. So did the issue go away in the meanwhile? Do we still want to commit this test ensuring this won't break again?

@hchonov, Sure. We added the field with this code in the module file.

/**
 * Implements hook_entity_base_field_info().
 *
 * Adds a boolean field to the node_field_data table.
 */
function MY_MODULE_entity_base_field_info(EntityTypeInterface $entity_type) {
  if ($entity_type->id() === 'node') {
    $fields['my_field'] = BaseFieldDefinition::create('boolean')
      ->setLabel(t('My Field'))
      ->setDescription(t('A boolean indicating when suggested terms are in the node.'))
      ->setDefaultValue(FALSE)
      ->setCardinality(1)
      ->setCustomStorage(TRUE);
    return $fields;
  }
  return NULL;
}

As you can see, the cardinality is set to 1. The field is a boolean whose only content can be NULL, 0, or 1 so we didn't think about translations, nor do I think we need to. The fact that this field is not revisionable is why my custom module is now broken. :-\

Per #11 here's a version of the reroll without changing $all_revisions to $use_revisions. Although since we're talking about using revisions, isn't it a good idea to keep the variable name change? Fwiw.

@tedbow, could you please confirm if the bug you've reported is still present? I think that the test from #16 is testing exactly this, but it isn't failing.

@esod, do you mind opening a new issue for your problem?

@tedbow, do you think that the attached test reflects the initial problem described here? If so, do you think it is useful to commit it or simply close the issue? I personally think the more tests we have in that the direction the better.

P.S. I think that the issue you've referenced - #2700261: allRevisions() entity queries ignore non-revisionable fields - has solved the problem indeed.

I think the we should explicitly state so the test doesn't get change in the future or if users because revisionable the test won't cover it anymore.

I can add this to the comments.

so actually is there test entity type that is explicitly not revision-able that we would join with?

This is already happening in the test, as it is adding a condition on the name of a referenced user entity and the user entity type isn't revisionable. Beside it there is the entity_test entity type, which isn't revisionable as well.

@hchonov, would you please explain how entity_test is not revisionable? Does your patch make that clearer? Does your patch make it clearer that the user entity entity type isn't revisionable? I'm curious to know.

Btw, I fixed my module. I changed ->setCustomStorage(TRUE) to ->setCustomStorage(FALSE) since I'm not using custom storage. I also added ->setRevisionable(FALSE) to be sure. I'd like my field to be revisionable, but it's not necessary for our current needs.

I didn't mean for my problem to be an issue for the community. I was just looking for answers and came across this issue. Thanks.

Trying out this patch on my jsonapi usage didn't fully fixed the issue. It still tries to join to the wrong table.
Here is a patch which helped with that.

Needs reroll for 8.7-dev

patching file core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
Hunk #1 succeeded at 190 (offset 16 lines).
Hunk #2 FAILED at 278.
Hunk #3 FAILED at 359.

Updating to correct status based on #30.

@rpayanm - Thank you for the reroll. It is advisable to remove the "Needs reroll" tag when the reroll is done.

Whoops, wrong issue! Sorry!

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.