Document interaction with the Trash module

Ouch, thanks for raising this. It's indeed very unfortunate that the Trash module is so aggressive when hiding trashed entities... This means that our code in the controller:

        $source_entity = $type_storage->load($source_id);
        if (!$source_entity) {
          // If for some reason this record is broken, just skip it.
          continue;
        }

will just think the source entity is broken since it can't be loaded, and skip it entirely from the table. I actually think this behavior from the Trash module could produce other unintended effects on sites using that module, because "the entity was moved into the trash" doesn't necessarily mean IMO "no other piece of code should be able to load this entity".

I'd like to give this a little more thought, but at this point I'm leaning towards just displaying a general warning message at the top of the usage page if the Trash module is enabled, or something along those lines. Thoughts?

I looked a bit deeper and I see that there seems to be a way to execute code bypassing Trash's hiding mechanisms. I don't really love the idea of special-casing Trash's behavior in our controller, but at this point maybe it's a good compromise?
So instead of having:

        $source_entity = $type_storage->load($source_id);
        if (!$source_entity) {
          // If for some reason this record is broken, just skip it.
          continue;
        }

we could have something along the lines of (untested):

        // Special-case the Trash module behavior, which hides the entities in
        // trash from all entity_loads.
        if (\Drupal::getContainer()->has('trash.manager')) {
          $trash_manager = \Drupal::getContainer()->get('trash.manager');
          $source_entity = $trash_manager->executeInTrashContext('inactive', function () use ($type_storage, $source_id) {
            return $type_storage->load($source_id);
          });
        }
        else {
          $source_entity = $type_storage->load($source_id);
        }
        if (!$source_entity) {
          // If for some reason this record is broken, just skip it.
          continue;
        }

and similarly to other places we may be checking things on the source entities (for example access checks, getting the labels, etc). If we know the entity is in trash, we may even add a suffix of type " (in trash)" to the label...

Thoughts?

@bkosborne sorry just re-read the issue summary and I am realizing you had already suggested something along those lines...

An alternative approach would be if this module detected if the Trash module was enabled, then it could deactivate trash's hiding feature on the usage page to it continues to show that usage, but then also indicates that the source entity is in the trash. All of that adds a pretty big coupling to the Trash module though.

So yes I believe we should go this route, even though I think it's suboptimal from a maintenance perspective... 🤷‍♂️

Okay, I'll work on this. I also polled some coworkers about this and they all agreed that it makes sense to show the trashed sources and to indicate they are in the trash.

Okay, test is failing because I was working with this patch for Trash 🐛 Trash's hook_entity_access should account for the 'view label' operation Active . Without that patch, trashed entities always show with a "- Restricted access -" label, even if the user has access to view trashed entities. It's a super simple fix there, so hopefully I can push that one through. But I'll postpone this issue on that.

Note there's also some phpcs failure but I don't think it's related to these changes.

Okay, the blocker from Trash is in. Just need to wait for its next release, then these tests should pas.

Okay, trash module released 3.0.17 which includes my fix. So I think our tests here should pass now. I'll re-run them.