🇸🇪Sweden @TwoD

Ignore #3, something went wrong when generating the patch so it does not apply.

The patch works for us, except for existing references already saved with the wrong langcode, those still cause a duplicate.
Added a simply workaround to just catch and log when it can't add a reference, instead of crashing with a WSOD.

The PR works well for us. Had issues with canonical node paths being marked as unrouted and bypassing the outbound path processing. Having them processed with the internal:// prefix where possible, makes more sense.

Tweaked the wording a bit, also made the interdiff against 11 to be more clear about all the changes since, again without white-space changes and test fixes.

I tried to be clever in the check for what would correspond to the default revision content and it already bit me, so modifying it and the message to better reflect reality.

#11 I think listing all revisions, instead of just the affected ones, is making it hard to tell how many changes there actually were to the translation being viewed. That could be a lot of revisions even with just two languages. Our use case is similar, with mostly Swedish nodes with some English translations here and there, but there can still be a large number of revisions.
I'm also not quite sure this actually fixes the issue, see below.

+++ b/src/Form/RevisionOverviewForm.php
@@ -209,15 +210,23 @@ class RevisionOverviewForm extends FormBase {
+        // If dealing with a translation, load that version.
+        if (($revision->hasTranslation($langcode) && $revision->isRevisionTranslationAffected()) ||
+          $langcode === $current_langcode || !$revision->hasTranslation($current_langcode)
+        ) {

This says it'll load a translation if one exists, but never actually does.

This condition also looks like it will always be true because $langcode is the entity's current language, which is by default the same as $current_language.

I took a slightly different approach and added the query filtering @recrit mentioned, which also meant a few conditions could be removed, and then added a check to figure out which older revision the current revision was based on, if it does not affect the viewed translation.
(I kept the hasTranslation check to be safe, since switching to one that does not exist throws an exception.)

The interdiff was generated without white-space changes to make the indentation changes easier on the eyes.

Correction for the format to use under "States based on values of multiple different fields" as arrays were nested wrong.

@ressa, I don't believe there is a reason the markup generated by any editor would lock you in, assuming the editor you are switching to is has the features/plugins needed to the type of markup you have. I mean, most of the editors are supposed to be usable to generate HTML for direct display - unless you're using one of the ones outputting other formats like Markdown.

First, a massive thank you to everyone involved here!

I've not yet had time to look into the work being done here, but do want you to know that I am willing to merge in new editor integrations. I just don't have much time to actively develop anymore, and it would be great if those creating the new integrations would be willing to stick around for a while after it has been merged. It is likely we will get new support requests, and maybe bug reports, as people migrate.

Please also note that all the older editor integrations have been deprecated now and in case Wysiwyg's core needs to change to support more modern editors, it is likely we'll drop support for those editors completely. If they happen to stay working for now, that's fine, but I don't really think they're that useful anymore - especially those that barely work with best practice markup or newer browsers.
So, we should no longer need to bend over backwards to not break the API (serverside or clientside) for newer editor integrations.

I'm not sure how I got it to fail with a shared table field like name earlier, maybe I had accidentally changed something in core I didn't notice.
The problem does appear there, but is masked by the shared field seemingly always being loaded from the revisions field table if it exists, while dedicated fields are not.

We have some fairly big entities and try to limit the number of revisions created for minor changes like typo fixes. We also have Content Moderation enabled, but override it to re-enable unchecking the "New revision" checkbox.

Our overrides now work well in most cases, except when you have added a paragraph in a Draft (non-default) revision, and try to update it without creating yet another revision. Then we basically run into the scenario from #11 (but instead of reverting to a previous revision we keep modifying the latest non-default one). The main change we've made is overriding NodeModerationHandler to not create a new revision, but still toggle published/unpublished - more or less what happens if saving a moderated entity in syncing mode.

This means the approach in #20 won't work for us, or anyone else not always using Content Moderation to always handle the default revision state.

It think the approach in #10 was closer to what we need, except it was missing the case when you add a new referenced entity to a draft of the parent, and then change the parent to be the default revision without creating a new revision.

The included tests first check that creating a new non-default revision does not make the referenced entity the default, preserving existing behavior, but it's also adding a new referenced entity which becomes the default revision because it's new.

Then it checks that updating the draft does not modify the default revision state of either referenced entity. This currently fails for the new referenced entity and causes the issue we saw.

The last part of the test "promotes" the existing draft revision to the default revision, and checks that both referenced entities are also promoted to being the default revision - ensured by the new condition added from the #10 patch.

TwoD → made their first commit to this issue’s fork.

Yeah, this really bites sometimes.

We've got a use case doing a lot of programmatic edits to individual paragraphs and had to come up with a helper like this instead of calling getParentEntity() directly:

The extra logging is there because we've hit all those cases with existing content which previously just called getParentEntity() and assumed it "did the right thing".

use Drupal\Core\Entity\ContentEntityInterface;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\Entity\RevisionableStorageInterface;
use Drupal\node\NodeInterface;
use Drupal\paragraphs\ParagraphInterface;

trait ParagraphHelpers {

  protected EntityTypeManagerInterface $entityTypeManager;
  
  /**
   * Get parent revision which actually refrences a paragraph.
   *
   * Paragraph parent references do not include the parent revision, so we may
   * need to check multiple parent revisions to find the one which actually
   * references the specific revision of the passed in paragraph.
   *
   * @param \Drupal\node\NodeInterface $node
   *   The top node.
   * @param \Drupal\paragraphs\ParagraphInterface $paragraph
   *   A paragraph.
   * @param string|null $langcode
   *   The language to load, default to the same as the paragraph.
   *
   * @return \Drupal\Core\Entity\ContentEntityInterface|null
   *   The parent entity revision referencing $paragraph, or NULL if not found.
   */
  protected function getParagraphParentRevision(NodeInterface $node, ParagraphInterface $paragraph, ?string $langcode = NULL): ?ContentEntityInterface {
    // Try the latest parent revision.
    /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $parent_storage */
    $parent_storage = $this->entityTypeManager->getStorage($paragraph->get('parent_type')->value);
    assert($parent_storage instanceof RevisionableStorageInterface, 'Can only handle revisionable entities.');
    $latest_parent_revision = $parent_storage->getLatestRevisionId($paragraph->get('parent_id')->value);
    $parent = $parent_storage->loadRevision($latest_parent_revision);
    /** @var \Drupal\Core\Entity\ContentEntityInterface $parent */
    $current_paragraph_delta = $this->getParagraphDelta($paragraph, $parent);

    // Try the default parent revision.
    if ($current_paragraph_delta === FALSE) {
      $parent = $paragraph->getParentEntity();
      $current_paragraph_delta = $this->getParagraphDelta($paragraph, $parent);
    }

    // Brute force fallback.
    if ($current_paragraph_delta === FALSE) {
      // Sanity checks.
      while ($parent instanceof ParagraphInterface) {
        $parent = $parent->getParentEntity();
      }
      if (!$parent instanceof NodeInterface || $parent->id() !== $node->id()) {
        $this->logger->error('Something is very wrong with paragraph @id (@rid) in node @nid (@vid)!', [
          '@id' => $paragraph->id(),
          '@rid' => $paragraph->getRevisionId(),
          '@nid' => $parent->id(),
          '@vid' => $parent->getRevisionId(),
        ]);
        throw new \LogicException('The passed in node is not the parent of the paragraph.');
      }
      // We may have a broken node structure if it comes to this, but at
      // least we'll be able to edit it and preserve the structure.
      $tree = $this->getFlatParagraphTree($node);
      $paragraph_data = $tree[$paragraph->id()] ?? NULL;
      if ($paragraph_data
        && $paragraph_data['revision'] === $paragraph->getRevisionId()
        && $paragraph_data['parentType'] === $paragraph->get('parent_type')->value
        && $paragraph_data['parentId'] === $paragraph->get('parent_id')->value
      ) {
        if ($node->isLatestRevision() || $node->isDefaultRevision()) {
          $this->logger->notice('Paragraph @id (@rid) in node @nid (@vid) was not referenced from the latest or default', [
            '@id' => $paragraph->id(),
            '@rid' => $paragraph->getRevisionId(),
            '@nid' => $parent->id(),
            '@vid' => $parent->getRevisionId(),
          ]);
        }
        /** @var \Drupal\Core\Entity\ContentEntityInterface|null $parent */
        $parent = $parent_storage->loadRevision($paragraph_data['parentRevision']);
        $current_paragraph_delta = $parent instanceof ContentEntityInterface ? $this->getParagraphDelta($paragraph, $parent) : FALSE;
      }
    }

    if ($current_paragraph_delta === FALSE) {
      $this->logger->error('Paragraph @id (@rid) in node @nid (@vid) was not referenced from any parent.', [
        '@id' => $paragraph->id(),
        '@rid' => $paragraph->getRevisionId(),
        '@nid' => $parent->id(),
        '@vid' => $parent->getRevisionId(),
      ]);
    }

    return $current_paragraph_delta !== FALSE ? $parent : NULL;
  }

  /**
   * Get at which delta in the parent field a paragraph is referenced.
   *
   * @param \Drupal\paragraphs\ParagraphInterface $paragraph
   *   A paragraph.
   * @param \Drupal\Core\Entity\ContentEntityInterface $parent
   *   The parent element.
   *
   * @return false|int|string
   *   The delta as an int/string or FALSE if not found.
   */
  protected function getParagraphDelta(ParagraphInterface $paragraph, ContentEntityInterface $parent) {
    $field_items = $parent->get($paragraph->parent_field_name->value);
    foreach ($field_items as $delta => $item) {
      // Explicitly compare ids first to avoid loading a new instance of the
      // referenced revision, which gives a clone since they are not cached in
      // storage, but may be cached on the parent's reference field item.
      if (
        (
          !$paragraph->isNew()
          && (
            $item->target_revision_id === $paragraph->getRevisionId()
            && $item->target_id = $paragraph->id()
          )
        ) || (
          $paragraph->isNew()
          && !isset($item->target_revision_id)
          && $item->entity
          && $item->entity === $paragraph
        )
      ) {
        return $delta;
      }
    }
    return FALSE;
  }

}

I got tired of restoring translations one by one, so I implemented @amateescu's suggstion from a while back and made it so restoring the original language by default also restores all translations - but they can be manually excluded if you don't want that.

Restoring an individual translation now also asks if you would like to restore other translations as well, but then the default is not to do it.

I've got nothing more planned for this right now so just awaiting feedback. :)

I based a relatively simple way to make Trash work nicer with Content Moderation based on ✨ Translation support Needs review as it already refactors entity operations - which should fix the issue in the OP - and it also refactors the storage trait to make this change easier.

Essentially it check if you're trying to trash or restore a non-default revision. If so, it performs the same operation on the existing default revision as well. That way if you have a draft and delete the entity, the published revision will become inaccessible.
If you then restore the entity it also restores the default revision along with the draft.
Because it already uses $entity->isSyncing(TRUE), Content Moderation will not attempt to change which revision is the default, or force a new one to be created.

It also decorates the moderation state validator to block any changes to the state while an entity is deleted.
Not had time to write any automated tests for this yet, but it seems to even work well with Workspaces (Core often creates links to blocked forms but I think's that's more to do with issues like 🐛 Views entity operations lack cacheability support, resulting in incorrect dropbuttons Needs work and 🐛 Operations links on the translations overview page do not link to the correct route Needs work ).

The PR against the translation branch makes it easier to see only the changes relevant here: https://git.drupalcode.org/issue/trash-3376216/-/merge_requests/1

I think this needs another look from someone other than me at this point.
As noted above, I pulled out all remaining revision/content_moderation related code. A bit late I noticed I had also pulled out the lines which set a new revision, but everything was still working as it should - and as @amateescu mentioned, this module should probably not enforce it unless strictly necessary anyway.

TwoD → created an issue.

Didn't implement restoring all translations at once yet, just wanted to get it back to passing the tests while awaiting feedback on the rest of my comments. At least I think it's working the way it did, a bit late here so didn't do much manual testing yet.

Maybe we can make restoring all translations at once optional? I mean when it's done via the trashcan, as it I don't think it make much sense when doing it via the translation overview.

Restoring a source language does not automatically restore all translations, but that is easily done from the entity's translation overview page.

We now have TrashStorageTrait::restoreFromTrash() (which wasn't available when this MR was created), can we restore all translations there? Or was there another reason for not doing it in the first place?

My reasoning at the time was that we don't easily know when a translation was deleted. If we force all of them to be restored it may unexpectedly restore some that were deleted much earlier, but I suppose the grouping in the trashcan should make that fairly easy to spot, unless you have a lot of languages. Either way, if the confirmation message explicitly lists the translations to be restored I would not be worried about that happening.

However, I would prefer if we then also had to explicitly pass in the translations to be restored to TrashStorageTrait::restoreFromTrash, and that it only saved the entity once even if multiple translations were passed in.

That it now explicitly saves the entities introduced some problems for this issues as it more or less broke the API.
The tests are calling trash_restore_entity($translation) multiple times before saving an entity, and that is no longer possible without creating multiple pointless revisions.
Granted, it was merged before 3.0 so I can forgive that, but it's a twist which means this will need some additional changes.

I'll see if I can implement that.

Isn't the real issue here that the menu link is still displayed even though it points to an inaccessible URL?

It's getting really late so I'm just pushed up a new fix based on what @Nixou did.

I noticed the query was very similar to what getLatestTranslationAffectedRevisionId() already does, but it also handles translations, which weren't always covered with the fix in #6.

I also had to wrap the new condition in a try/catch because of what happens in FieldableEntityDefinitionUpdateTest. The entity type is in the process of being updated to say it's revisionable, but the tables don't seem to exist (yet?). Maybe the try/catch is hiding another bug, but I don't have time to investigate that further right now.
Could use an extra pair of eyes if anyone has the time, so marking NR.

Sigh...

It was easier just to use the new field in the modified test class.

This patch modifies one of the existing revisioning test classes to include changes to a multi-value field (stored in a dedicated table), which is required for the SQL content entity storage implementation to run the code affected by the described issue.

The use case presented here aims to cut down on the number of required revisions to be created, especially when no fields are actually modified. I have the need for the same use case as the number of revisions created for minor changes can get excessive, and this would be a simple way to avoid some of those.

The modified test runs a sequence of changes to an entity, creating a new revision each time, to check that certain types of field modifications across translations are either prevented through validation errors or correctly stored. This is similar to having the Content Moderation module active for the entity as it forces revisions to be created for every modification performed via the entity form, or the separate publish-only form.

The changes add parameters for preventing the new multi-value field from being modified, and preventing a new revision from being created.
The createRevision() method is still invoked to copy how the entity form sets up the entity, and if no new revision is desired the test simply calls $entity->setNewRevision(FALSE). This also preserves the setup of $entity->original so field modifications are accurately detectable.

The test data provider adds two steps which first create a new pending Italian revision with changes to all the fields, and then turns that latest revision into the new default revision without any other changes.
This triggers the observed behavior in the SqlContentEntityStorage class where it skips writing the latest revision's field values into the main table for the entity, leaving them only in the field's revision table, because it does not detect an actual change.
This does not happen for the label field or the untranslatable field as those are handled by the shared table method, which does not have the same "optimization".

Note, I added the new multi-value field to the test entity for simplicity, but if the slight additional overhead is considered too much for other tests using the same entity it could be added in the test class itself.

@EZKG, it should already be handing translations in that way, and there are tests included to verify that, but I can take another look soon.

There have been multiple changes since my version and the tests no longer pass so I'll need some time to dig into what's going on.

Ah, I missed that the spellecheck plugin was completely removed from the build, that does need a migration path.
Could be useful to clearly highlight the need for (and allow the input of) the license key.
Will open a new PR for this to get some input.

Thanks!

TwoD → made their first commit to this issue’s fork.

Good point. I mentioned it in the release notes but in the actual instructions would be better. I'm not able to do it this weekend but if you want to contribute a patch, open a new Docs issue.

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

I think someone may be pulling a prank on you. Response code 418 is "i'm a teapot". Wysiwyg only responds 200 or 403 from the wysiwyg_filter_xss_page_callback() controller function.
You may need to look into what else could be intercepting the request on that path.

Thank you all.

Thanks all!

TwoD → made their first commit to this issue’s fork.

This patch no longer applies.

Committed ✨ Update to CKEditor 4.21.0 Fixed .

Thank you all for this. I bumped it all the way to 4.23.0 as that is an LTS and there were no breaking changes directly affecting Wysiwyg.
Expect a release soon.

Initial attempt at fixing this. My Views no longer crash and by default they only include non-deleted entities.

I'm not very used to altering Views queries so could use some help making sure this is correct.
I noted Trash does not modify entity queries if you tell them to look at all revisions, but I think it makes more sense to actually do this for Views.

This could use some tests with revision based Views as well.

TwoD → created an issue.

I am using Trash 3 with the patch from ✨ Translation support Needs review and Content Moderation without any issues like the one described here.
Our node translations have independent moderation states and the patch also allows for [mostly] independent trash states. (Deleting the source translation marks all translations as deleted, and restoring a translation requires that the source translation has also been restored.)
Any help reviewing that would be much appreviated. It may even be benificial if you don't need the actual translation support since your review could confirm that use case is still working, but it also improves revision handling.

I do however have an unrelated issue with revisions at the moment that I'm looking into. Specifically it's when a View is based on say the node revision table instead of the main node table (or pulls in the node revision table). Trash gets a bit confused about which "deleted" field to check and uses the main table name in stead of the revision table name, breaking the query. If I get any further with that I'll post it as a separate issue.

These changes improves the GUI somewhat and better track what happens when you normally delete an entity.
Translations can now be independently deleted, restored or purged without affecting the source language from the language overview page.
Deleting or purging a source language also affects all translations.
Restoring a source language does not automatically restore all translations, but that is easily done from the entity's translation overview page.

The new revisions when deleting are enforced to be the default revision, or you could end up with a translation that could not be edited because the latest (draft) revision was deleted but the default revision was still published.
Content Moderation may have "opinions" on when this is allowed, but since we have the storage trait we can ensure Trash gets the last word. Maybe we should even set the "is syncing" flag for these operations?

One side effect of that is that restoring something where the latest revision was a draft (unpublished) and the default revision was published, will not actually be published after a restore - as the default revision is now the unpublished draft (without the deleted timestamp). You would have to fix that either by publishing the draft or by restoring a previous revision as the default manually.

Toyed with the idea of removing the latest revision completely instead of just clearing the deleted timestamp (for revisionable entities), but ran into complications because you can't delete a default revision and it made me question if it was reasonable to always set the revision before the deleted revision as the new default before removing the "deleted" revision.

This could use some functional tests for the GUI changes in the translation overview and the trash listing itself, but I figured it'd be best to get some feedback first before spending that time in case big changes are requested (or this is rejected completely).

No specific CKEditor 4 version is not bundled with Wysiwyg module and there is nothing preventing anyone from installing 4.18 or any newer version.
The message you see if you do install a newer version really only means we can't guarantee an update does not need a migration path to automatically reconfigure the editor profile to compensate for changes outside the current range. The CKEditor team have been very good at keeping with the promises of semantic versioning and 4.x has been very stable for a long time so I doubt anyone would actually have any issues.

Admittedly, I could have done a better job of publishing new versions of Wysiwyg module, but since it's just a warning message I've not considered it critical so far.

I'll see if I can get some time this weekend and pull together some cleanup patches for a new release, including bumping any "supported version" ranges as needed.

Many thanks again to @steinmb, who is plowing through the issue queue, reminding me there are still things to do here, and that it would not be wasted time to keep D7 contrib modules rolling.

Agreed. I barely have time to maintain the current integrations (most of which are practically dead) so I don't see a reason to include another one. If someone is using this they have already either implemented Wysiwyg's API in a new module or forked Wysiwyg itself.

And, thank you @steinmb for the issue queue pruning, very much appreciated!

I think something sounds wrong if the ckeditor-3.0.js script gets downloaded multiple times. Drupal sends along a list of already loaded scripts in AJAX requests to the server and it should not be including those in the response.
Could you please provide some more info on the setup and how the lightbox is openened?
Maybe there's some edge case I've yet to see. If so, that could mean there are more places where this may need to be accounted for (like other editor integrations or the internal Wysiwyg state tracking field instances).

Just discovered I forgot a few calls so it restores the original translation instead of the removed one.
Will see if I can adapt the tests to catch that as well.

+++ b/src/Plugin/EmailAdjuster/InlineCssEmailAdjuster.php
@@ -47,11 +63,17 @@ class InlineCssEmailAdjuster extends EmailAdjusterBase implements ContainerFacto
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, AssetResolverInterface $asset_resolver) {

Please don't commit this without providing backwards compatibility, we've already been bit (multiple times) by this module making breaking changes in minor versions.

This particular case may not be the most likely class to have been subclassed by users, but you can pretty much guarantee someone has done it. Sometimes you can guard a bit against constructor changes by only overriding the create() method in your subclass and tack on the extra dependencies there, but it's no longer true DI and makes it harder to test since now you need tests to create the DI container and register the dependencies there instead of just passing them to the constructor.

At least let the new arguments default to NULL, fill in from \Drupal::service() if not set, and trigger a deprecation warning instead of just expecting them to be set.

The schema update for the new setting did not make it into the previous patch.

I re-implemented this serverside and added a checkbox to the widget settings to toggle alphabetization of the render list without affecting which buttons are made available for quick access - the drag & drop weight still controls that.

This is for the 10.x version as that was the only one working for us with the latest Paragraphs and Paragraphs Features modules.

Lol, was just about to check in on this and it's already RTBC.

Do we need an actual test to ensure broken scripts keep making it through to the aggregated files (or at least go through this part unmodified)?

Oh, right, that change may make the module incompatible with anything but D10+.

TwoD → made their first commit to this issue’s fork.

TwoD → created an issue.

Whoops, meant to create this for 11.x, but no matter, they seem to be identical.

Pushed up a simple proof of concept which catches the exception, creates an informative log entry if it can, and proceeds to use the original source to not cause a complete crash.

Other than it also needing a test, Is this a good approach?

TwoD → created an issue.

This issue was a major fuckup, breaking deploy scripts and existing workflows without warning because some people are somehow offended by an acronym?
What's so damn inappropriate that sex can not be apart of even an acronym? You ARE CREATING issues with "inappropriate words" specifically by doing things like this.

The fact that csex (AND csim!!!!???) was removed in some versions, there was no deprecation period, no replacement alias (at least not in 2.0.0-rc4), and the change was made for this specific reason is astonishingly stupid and an embarrassment for the maintainers!
Had I seen this before we changed over to the 2.x branch I would rather have dropped the module completely than supporting such weak and unreliable maintainership.

I would have been ashamed to propose a change like this and am now instead ashamed of being a part of a community which bent to such petty requests - achieving nothing but wasting countless developer's time over nothing but someone deciding sex offends them.
What if I say "drush" offsends me because "ush" in Swedish has a negative association? Or that we should not even use "association" because it contains "ass"? See how moronic this gets if you even let it start?

Yes, this will need some more rebase work once the issues mentioned in #115 are done.

The changes to the Renderer have been committed to the 11.x branch, but not the 10.x or 10.1.x branches, so if you are using D10 you are likely missing the corresponding change from that issue - which is likely to cause problems like those you describe as cache metadata is lost in some situations.

Went with just triggering a warning. Should be "loud" enough to be noticed but not crash things.

@Wim, hey! Long time indeed, maybe we'll meet in Lille?

My opinion is that if someone manages to trigger the code path which does not throw an exception today they're already in dangerous territory but it is "working" because a safeguard is missing.

As catch pointed out:
They would have to be rendering a structure where one part ends up not being accessible, which is of course common and fine on its own, but there can't be a render context available - as then it would be triggering exceptions when access is allowed to any other part of the same structure. As far as I know we don't expect the renderer to ever run without a render context available, hence the exceptions.

I can't think of a useful situation which would be relying on an exception not being thrown when rendering something completely inaccessible without a rendering context and.
If we don't throw an exception here, what's the alternative? Don't bubble the metadata if we don't find a context?

TwoD → created an issue.

+++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
@@ -83,7 +83,7 @@ public function form(array $form, FormStateInterface $form_state) {
-    if ($this->isCanonicalMode($this->entity->getMode())) {
+    if (TRUE || $this->isCanonicalMode($this->entity->getMode())) {

Leftover debug override?

The same exception would be thrown today if access is allowed and the element does not have #printed yet, so I don't think that would happen, or do you have a specific case in mind?

I cherry-picked/rebased on top of 10.1.x and all tests pass locally [except for the ones I normally can't get to pass at the moment] so let's see what the bot says.

There was a [previosuly ignored] phpstan error related to Renderer::getCurrentRenderContext() about the return value not being nullable but the renderer using if (!isset($context)). The result can actually be null, so I removed the ignored error and corrected the type hint.

This now depends on 🐛 Content translation access manager may use the wrong revision Needs work and 🐛 The renderer throws away cache metadata from access result if it is not allowed Fixed .

TwoD → created an issue.

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.

TwoD → created an issue.

Thanks, working on that now.

Do you know what's up with the bot? This is my first time I see it so trying to figure out what it's trying to tell me.
The patch applies fine against 11.1.x which it wants to merge into, but I guess it's testing against 11.x?

There is indeed a test which validates that all cache backends have case-sensitive cache ids in GenericCacheBackendUnitTestBase, but what makes it case-[in]sensitive is (obvious in hind-sight), the selected collation for the cid column.

We were using utf8mb4_sv_0900_ai_ci, and switching to utf8mb4_sv_0900_as_cs for just that column on the cache tables fixes the issue with notices for us.

We get the same type of error when page cache stores the results of a view with an exposed filter of some value, and you filter again for the same string with a different combination of uppercase/lowercase characters.

Page cache generated a cache id with the first search string and stored that in the cache table via the database backend.
The next time page cache asks for the same string (only case differs) the database backend performs a case-insensitive query and returns the original item, which has a different cache id compared to what it put in the mapping array.

IIRC MariaDB, MySQL and SQLite all do case-insensitive queries by default, unsure about PostgreSQL, so maybe it would be easiest to make the database backend just lowercase the cids when normalizing them.
Other cache backends, like memory, will of course treat these as different cache ids. The interface does not explicitly mention how this is intended to work so maybe we can get away with that?

Now that all tests finally pass, I should write some notes on the changes outside the implementation of #108 itself.

There were at least two major issues discovered while working on this which could potentially be extracted and handled on their own.
I tried searching the issue queue for existing patches and came up blank, but maybe someone knows if they have already been covered elsewhere.

The renderer throws away cache metadata from access results if they are not allowed. This, obviously, prevents something like #108 from working; we need that metadata to be bubbled the same was as if there was a cache hit/miss when access is granted. Without this the new ::testEntityOperationsCacheability() fails because the user cache context is dropped.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -233,6 +233,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
       if ($elements['#access'] instanceof AccessResultInterface) {
         $this->addCacheableDependency($elements, $elements['#access']);
         if (!$elements['#access']->isAllowed()) {
+          // Abort, but bubble new cache metadata from the access result.
+          $context = $this->getCurrentRenderContext();
+          if (!isset($context)) {
+            throw new \LogicException("Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead.");
+          }
+          $context->push(new BubbleableMetadata());
+          $context->update($elements);
+          $context->bubble();
           return '';
         }
       }

(Added null to the annotated return types of ::getCurrentRenderContext() as that was missing.)

The content translation manager sometimes looks for existing translations on the wrong revision.
This was exposed by ContentTranslationRevisionTranslationDeletionTest::testOverview() failing because the 'Add' operation was now missing for Italian on line 142. They still had access to the translate form if the current interface language was either Italian or French, but not in English. While the Add operation [correctly?] points to the Italian version of the translation form it being added depended on an access check performed on the "active" revision - which already has an Italian translation - so the link no longer show up because the URL is inaccessible.

The test performs these steps:
(This is iteration 2 of the test, with the editor role. Iteration 1 passes because access checks are bypassed.)

1. Create an English published node "Test 2.1 EN"
   Verify translations can be added.
2. Add an Italian draft "Test 2.2 IT".
   Verify the draft can not be deleted because it's unpublished.
3. Publish the Italian translation "Test 2.3 IT"
   Verify it can be deleted.
4. Create an English draft "Test 2.4 EN".
   Verify Italian translation still exists.
5. Delete the Italian translation. Verify the 'Add' operation for Italian reappears.
6. Publish the English draft "Test 2.6 EN".
   Verify the Italian translation does not reappear.
7. Add an Italian published translation "Test 2.t IT".
   Verify it can be deleted.
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.
10. Delete the Italian translation.
    Verify it can be added again.

When the Italian translation is deleted in step 5 the English draft "Test 2.4 EN" is the latest revision and no longer has a translation. The access manager gets its argument from the route match for the translation route, which has load_latest_revision = TRUE, but it loads the latest "active" revision - which the entity repository says is the last one which was translation affected.
The last English revision which was translation affected (titled "Test 2.1 EN") still had "Test 2.3 IT" as a translation, as the deletion created a new revision without any translations affected.
ContentTranslationController::add() deals with this by explicitly loading the latest revision, so we can do the same thing in the translation access control manager and it correctly sees that there is no Italian translation there and allows the 'Add' operation.

The controller does however cause a similar issue elsewhere by adding the translation to the entity it loads (after the access control manager has allowed the route). If you have the language switcher block enabled the links there are now access checked via the same manager, and it now sees the newly added translation (which isn't considered "new" since an Italian translation did exist earlier) and prevents the links from showing.
Adding a clone to the controller prevents polluting the entity instance in the context, and the subsequent access checks then see the true "current" state of the entity and lets the language switcher links show.

Other changes made to make tests pass include adding _access: 'TRUE' to the <current> route, which caused a problem in CommentNewIndicatorTest<code> because <code>CommentLinkBuilder uses <current> for the "comment-new-comments" hidden jump link and renders it as a link list.
I can't think of any reason why this would not be safe off the top of my head, but there's also no other test which directly depends on it it being either allowed or denied.

A few places in FunctionsTest had similar issues with the access checks but some routes could be switched to existing ones with correct access requirements or replaced with a new one.

Thanks! Looks good to me and very similar to what I tested locally before reverting to the release.

TwoD → created an issue.

TwoD → created an issue.

A patch equivalent to the current PR was applied as part of 🐛 Code error url fragment: wrong array key: key #fragment should be fragment Fixed , including test coverage.
#6 was not addressed though.

You need to do something like this:

/**
 * Implements hook_form_alter().
 */
function MYMODULE_form_alter(&$form, &$form_state, $form_id) {
  $form['#pre_render'][] = 'MYMODULE_form_pre_render';
}

function MYMODULE_form_pre_render($element) {
    $group = 'THE_GROUP_NAME';
    $element[$group]['#states'] = THE_STATES_ARRAY;
    // #states requires an element id, which groups don't have by default.
    $element[$group]['#id'] = $group;
    drupal_process_states($element[$group]);
  }
  return $element;
}

For D7 this also needs to test just - as inputting just that currently leads to
PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '-' for column 'FIELD_NAME' at row 1: INSERT IN ....

TwoD → created an issue.

We already have cache metadata for the operations indirectly through their URLs, but most list builders [and the Views Operations field] simply throw it away after evaluating the access condition (at operations build time), so the context of when an operation could be valid or not is lost.

I think this is what the failing test is actually hinting about, a responsibility creep. To fix the failing test I would have assumed the access control handler for the "disable" operation was not returning the proper [user] context. However, adding it there has no effect at all on whether the test passes or fails since that data is thrown away by the list builder when it check if access is currently allowed.
(We try to avoid early rendering, maybe we should also try to avoid early access evaluation in similar situations?)

I ran into this when debugging specifically why our cache contexts added to custom entity access control handlers seemed to have odd effects despite the value of the context(s) we included was different for different users, even after applying the latest patch - because contrib's entity module's list builder has the same flaw as core's list builders currently do.

If the list builders (and Views' Operations field) always added the operations instead of evaluating access at the point of building the operation links, and the links preprocess template hook performs the actual access evaluation (storing the result in $link['#access'] and merging in the cache metadatada into $link['#cache']), we'd be all good.

This would keep access control handlers as the ones responsible for adding the correct access related cache metadata to their responses instead of also having list builders juggle it around, and relieve them of even having to do the operations access checks they do now - they would not have to care about it all about which operations are valid or not.

For now I've had to resort to always include a custom cache context (which varies with anything that could affect access for the different groups of users we have) in the links preprocess hook, otherwise the operations access in the lists/views we have would not get rebuilt at all. If it worked as described above (and entity module always included all of its operations) I would not have had to do anything in our site specific code because the cache metadata from the access handler would have been propagated there instead.

I'll take a stab at refactoring the branch to show what I mean.

The D7 patch in #358 DOES NOT work. It created massive problems for us by removing all ON - conditions in joins to aliases of {node}. Views which relied on node access show random nodes when the join condition is cast to a string (the only real difference since #322).

#332 is still the way to go. We've been using it since 7.52 and confirmed it still does the same thing on 7.54 without issues.

@ExTexan, when/where did you get that error?

@tonka67 The only output I got from git was

<stdin>:455: trailing whitespace.
          // Non-empty nodes are wrapped in <a> solely for sake of xpath counting --
<stdin>:465: trailing whitespace.
      }
Warning: 2 rows add whitespace errors

The patch still applies cleanly, it's just that git doesn't like the whitespaces it adds after a comment and the closing curly bracket. It's in node_access_test.module so wouldn't affect the normal functionality.

EDIT: Removed extra output from the curl call I piped into git...

We are using #332 in production (on PHP 5.6 and MySQL). Picked it for the reasons outlined in #330. Basically, it's a very useful API addition, it's backwards compatible, it comes with tests, and it feels like something which D8 should handle in the same way.

Had no issues applying #332 to 7.51, other than a couple of extra whitespaces. Verified it still works for our test cases.

Our test case includes a user which should be able to see certain nodes in a View, which may optionally refer to another node of the same type (its "parent"). Which node(s) the user has access to is determined using node access records/grants and they normally get access to both the "parent" and the "child" nodes.

Without the patch in #332, our user is not able to see any of the top level nodes which do not have a parent. Applying the patch immediately makes the top level nodes appear in the View, with the "parent" left empty, as they should.