Content translation access manager may use the wrong revision

I looked around a bit and found two issues which sound related, but the fixes do not appear to overlap that much. Could possibly need to adjust the changes depending on what gets committed first.

Leaning on the test coverage for this one, which does fail without the fix.

Think this is ready for committer review.

I am doing triage on the core RTBC queue → .

@TwoD, thanks for the complete issue summary and MR.

I read the Issue Summary which has great detail, thank you. The proposed resolution is also clear. The steps to reproduce have references to the UI which makes we wonder if before/after screenshots are needed to help reviewers. But then again, maybe that isn't needed if this has some manually testing.

I read the comments and I do not see a code review.. Setting to Needs Review for that.

Since this involves both content moderation and multilingual I am tagging for subsystem maintainer review. I also think this should have some robust manual testing, adding tag.

I did not review the patch or test it.

I forgot to add that I added some line breaks to the Issue Summary to make it easier for me to read.

Restoring status as this seems to have stalled after 2 months.

Rebasing the issue and running the test-only feature I correctly get

There was 1 failure:
1) Drupal\Tests\content_translation\Unit\Access\ContentTranslationManageAccessCheckTest::testCreateAccess
Expectation failed for method name is "moduleExists" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.
/builds/issue/drupal-3374234/vendor/phpunit/phpunit/src/Framework/MockObject/Matcher.php:237
/builds/issue/drupal-3374234/vendor/phpunit/phpunit/src/Framework/MockObject/InvocationHandler.php:174
/builds/issue/drupal-3374234/vendor/phpunit/phpunit/src/Framework/MockObject/Api/Api.php:86
/builds/issue/drupal-3374234/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-3374234/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3374234/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3374234/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3374234/vendor/phpunit/phpunit/src/TextUI/Command.php:97
FAILURES!
Tests: 1, Assertions: 2, Failures: 1.

Looking at the solution core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php seems to match the proposed solution of the IS.

Added screenshots to the testing steps up until the problem appears.

I'm not clear on steps 8 and 9 in the steps to reproduce, they seem to contradict each other

8. Create an Italian draft "Test 2.8 IT".
Verify it can not be deleted.
9. Delete the Italian draft.
Verify it can be deleted again.

In step 8 we verify it can't be deleted, but in 9 we delete it - is there a typo?

Left some comments on the MR

I think this will need eyes from someone more familiar with the entity system and languages than me, which we already have it tagged for.

The access manager should use the latest revision when looking for existing translations, not the "active" revision, as already done by ContentTranslationController::add()

the referred to add method calls

$storage->getLatestTranslationAffectedRevisionId

and not the latest revision as in your code

storage->getLatestRevisionId

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/conte...

the add method comment explains why this check is needed and it is very different reason than what you describe here

otherwise the initial form values may not be up-to-date.

i believe there is no logical connection between the add method and what you are trying to resolve, completely different scenarios .

the access handler should not decide on which revision to load, this is decided by entity repository getActive method .
the whole purpose of getActive is to make such decisions, so it is wrong to say getActive is not latestRevision because sometimes it is equal and sometimes not .
when getActive decides it must divert from the latestRevision, it is making this decision for good reason and any other function must respect this decision .

what you suggest is this : getActive have decided NOT to return the latest revision, but after you have already defined your route to call getActive then you override its decision and do enforce loading the latest revision .

you must not debate what access manager should result in, you need to debate what getActive needs to return under given scenario .
your description text that evolves around access checking makes it confusing to understand the underlying issue .

Thank you for taking the time to look at this and the feedback!
I have tried to clarify a few things in the original post. It's complex to explain and a bit convoluted to get the config right to be able to reproduce this, so forgive me for confusing things a bit.

Yes, getActive() may be the correct place to fix this, but right now I don't see how it could make the correct decision.
What it loads is the "latest translation affected revision", which in the test case becomes an earlier revision than the very latest revision, as the very latest revision does not have an Italian translation.

The access handler will not give you permission to add a translation, because as far as it knows there is already an Italian translation at that revision (and always will be).
The issue arises after deleting a translation when there is also a future revision which did not affect the translation being deleted.
Only by explicitly loading the very latest revision - regardless of the active UI language, which is what getActive() makes its decision based on - can we know if it should be possible to add a translation or not.

We won't ever check if a translation can be created for a past revision, as revisions are considered "read only", so to me it seems to make sense to always explicitly load and check access using the latest one.

Regarding $storage->getLatestTranslationAffectedRevisionId() vs $storage->getLatestRevisionId(), I made a mistake there and have added a note about it in the OP as well. Leaving ContentTranslationController:add() as it is with >$storage->getLatestTranslationAffectedRevisionId() leads to a 403 if one tries to add an Italian translation when the UI language is English (/node/1/translations/add/en/it), but works if the UI language is anything else (/it/node/1/translations/add/en/it), which seems odd.
Adding the proposed change leads to (/node/1/translations/add/en/it) instead crashing because it's trying to add Italian to an entity revision which already has italian. But if you switch the UI to Italian it still works, because the loaded revision is the default revision and it therefore doesn't try to load the latest translation affected revision for Italian.
If I change ContentTranslationController:add() to also use $storage->getLatestRevisionId() it works, but I admit I have not had time to fully test the consequences of that.

If there is a way to fix this in getActive() I'd be happy to try that, but right now I don't see how we from inside there could tell that we're not actually interested in the "latest affected Italian revision", when at most other times we do want that.
Maybe load_latest_revision=true isn't enough on that route and we need a flag like ignore_translation=true?

Am I completely wrong in thinking that we should always be looking at the very latest revision when deciding if a translation can be added or not?

Have I misunderstood something about how the current UI language should [not] affect which translation(s) can be added? (It seems to me it should not matter if I have the UI in English or Swedish when adding a German translation if I speak all of them, but prefer the UI to be a specific one for familiarity.)

the `_access_content_translation_manage` is used in multiple routes under `Drupal\content_translation\Routing\ContentTranslationRouteSubscriber`
changing the loaded revisions like you suggest will apply also to operations `edit` and `delete` which is not correct

if we do decide that indeed `load_latest_revision` is not working correct, then the change needs to happen at route definition by changing the ParamConverter from 'entity:node' to 'entity_latest:node' which will be custom converter extending `Drupal\Core\ParamConverter\EntityRevisionParamConverter` .

however, it seems to me that the problem not in behavior of getActive but the wrong implementation of revision-delete which creates new revision instead of actually deleting the revision trail like a pop stack operation.

alternatively if we do decide to go the path of changing getActive (which i think the better approach)
then you can add condition here :
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
after it checks $revision->wasDefaultRevision() && !$revision->isDefaultRevision()
set it is to null and it will return the latest revision .

  if ($entity instanceof TranslatableRevisionableInterface && $entity->isTranslatable()) {
      /** @var \Drupal\Core\Entity\TranslatableRevisionableStorageInterface $storage */
      $revision_id = $storage->getLatestTranslationAffectedRevisionId($entity->id(), $langcode);

      // If the latest translation-affecting revision was a default revision, it
      // is fine to load the latest revision instead, because in this case the
      // latest revision, regardless of it being default or pending, will always
      // contain the most up-to-date values for the specified translation. This
      // provides a BC behavior when the route is defined by a module always
      // expecting the latest revision to be loaded and to be the default
      // revision. In this particular case the latest revision is always going
      // to be the default revision, since pending revisions would not be
      // supported.
      $revision = $revision_id ? $this->loadRevision($entity, $revision_id) : NULL;
      if (!$revision || ($revision->wasDefaultRevision() && !$revision->isDefaultRevision())) {
        $revision = NULL;
      }
    }

the root problem here is with the delete translation operation .

for me in project which i worked on, we have to chosen to implement different translation delete operation which pops out the whole stack of draft revision for a given language translation until it reaches the published latest affected translation or continue to the first ever revision

Hmm, I'll have another round with this and try some of those things out. We've not noticed any issues with the update/delete operations during the year this has been in production for us, but probably because the UI does not allow to do any language operations other than on the top revision and we have no custom code affecting earlier revisions.

In the case here the "top" revision is a draft made in English, and then the test deletes the Italian (published) translation.
Popping off the first revision would remove the English draft, since revisions aren't separate per language, and continuing down would just cause more chaos? Does it ignore that top revision because it only affected English, and completely deletes earlier revisions only if they were marked as affecting Italian, even though technically others do have a copy of the Italian contents in them too?
That sounds like it also makes it impossible to revert any translation deletions, even through custom code, which should be possible now since no revisions are deleted when deleting a translation.
We also have to consider things like contrib's Trash module which intercepts the delete translation op, reinserts the deleted contents into the new revision and just adds a "deleted" flag instead. If no new revision is created when a translation is removed, that would not be possible (or Trash module would have to reintroduce that behavior, and we're back to this problem).

It feel strange to delete revisions in a delete translation op given how the rest of the system treats revisions (basically immutable until a user explicitly deletes them), but maybe it would be clearer if you had a sample of what that customized operation does?

here is my code to delete previously published draft translation .
i welcome your feedback if indeed this creates problem in reverting .
for each revision along the stack three options :

1. there exists other affected translations, then only remove the intended language and sync revision
2. there aren't other affected, then delete the whole revision
3. if the target deleted translation is also original language then we can not delete it since it holds the untranslatable fields

namespace Drupal\content_administration\Form;

use Drupal\node\Form\NodeRevisionDeleteForm;
use Drupal\Core\Form\FormStateInterface;
use Drupal\node\NodeInterface;
use Drupal\views\Views;
use Drupal\Core\Entity\EntityChangesDetectionTrait;
use Drupal\content_administration\WasPublishedTrait;
use Drupal\Core\Language\LanguageInterface;

class NodeDraftStackForm extends NodeRevisionDeleteForm {

  use EntityChangesDetectionTrait;
  use WasPublishedTrait;

  public function getFormId() {
    return 'node_draft_stack_confirm';
  }

  public function getQuestion() {
    return $this->t('Are you sure to delete all unpublished revisions?');
  }

  public function buildForm(array $form, FormStateInterface $form_state, ?NodeInterface $node_revision = NULL, ?LanguageInterface $target_language = NULL) {
    $this->target_langcode = $target_language->getId();
    $this->wasPublishedStack($node_revision);
    $form = parent::buildForm($form, $form_state, $node_revision);
    return $form;
  }

  /**
   * {@inheritdoc}
   */
  public function submitForm(array &$form, FormStateInterface $form_state) {
    $revisions = $this->loadRevisionStack();
    $node_published_default = NULL;
    $languages = \Drupal::languageManager()->getLanguages();
    $target_language_name = $languages[$this->target_langcode]->getName();
    $messages = [];
    foreach ($revisions as $node_revision) {
      $affected = 0;
      foreach ($languages as $code => $language) {
        if ($this->target_langcode != $code && $node_revision->hasTranslation($code)) {
          if ($node_revision->getTranslation($code)->revision_translation_affected->value) {
            $affected++;
          }
        }
      }
      if ($affected > 0) {
        $node_revision_target = $node_revision->getTranslation($this->target_langcode);
        if ($node_revision_target->isDefaultTranslation()) {
          $this->resetOriginal($node_revision, $messages);
        }
        else {
          $this->removeTranslation($node_revision, $messages);
        }
      }
      else {
        $this->deleteRevision($node_revision, $messages);
      }
      $this->logMessages($messages, $node_revision, $target_language_name);
    }
    $this->redirectSubmit($form_state);
  }

  protected function loadRevisionStack() {
    $query = $this->nodeStorage->getQuery();
    $query
      ->accessCheck(FALSE)
      ->allRevisions();
    $query
      ->condition('nid', $this->revision->id())
      ->condition('vid', [$this->stack_published, $this->stack_top], 'BETWEEN')
      ->condition('langcode', $this->target_langcode)
      ->condition('status', 0);
    $vids = $query->execute();
    return $this->nodeStorage->loadMultipleRevisions(array_keys($vids));
  }

  protected function deleteRevision($node_revision, &$messages) {
    $this->nodeStorage
      ->deleteRevision($node_revision->getRevisionId());
    $messages['log'] = '@type: deleted @language revision %revision.';
    $messages['status'] = 'Revision %revision has been deleted.';
  }

  protected function removeTranslation($node_revision, &$messages) {
    $node_revision->removeTranslation($this->target_langcode);
    $node_revision->setSyncing(TRUE);
    $node_revision->save();
    $messages['log'] = '@type: removed translation @language from revision %revision.';
    $messages['status'] = 'Translation @language has been removed from %revision';
  }

  protected function resetOriginal($node_revision, &$messages) {
    $node_published_default = $this->nodeStorage->loadRevision($this->stack_published);
    foreach ($this->getResetFieldnames($node_revision) as $fieldname) {
      $field_value = $node_published_default->get($fieldname)->getValue();
      $node_revision->set($fieldname, $field_value);
    }
    $node_revision->setRevisionTranslationAffected(FALSE);
    $node_revision->setSyncing(TRUE);
    $node_revision->save();

    $messages['log'] =
      '@type: reset @language from revision %revision to published state.';
    $messages['status'] =
      'Original @language from %revision has been reset to published state';
  }

  protected function logMessages(&$messages, $node_revision, $target_language_name) {
    $this->logger('content')->info($messages['log'], [
      '@type' => $node_revision->bundle(),
      '@language' => $target_language_name,
      '%revision' => $node_revision->getRevisionId()
    ]);
    $this->messenger()
      ->addStatus($this->t($messages['status'], [
        '%revision' => $node_revision->getRevisionId(),
        '@language' => $target_language_name,
      ]));
  }

  protected function getResetFieldnames(NodeInterface $node) {
    static $reset_fields = [];
    if (!$reset_fields) {
      $skip_fields = $this->getFieldsToSkipFromTranslationChangesCheck($node);
      $skip_fields[] = 'changed';
      $reset_fields = array_diff(array_keys($node->getFieldDefinitions()), $skip_fields);
    }
    return $reset_fields;
  }

}