Delete job item when corresponding entity is being deleted

A first feedback here:

A translator plugin needs to be made robust enough to deal with missing items. We already had many small related fixes.

The idea was that we want to remember what we submitted to the provider.
I would prefer to keep it and maybe mark it stale if we know it's outdated.

Thank you for the proposal, we will need to discuss this direction.

Note also that core also doesn't support such a thing:
If you delete an entity B that is referenced from entity A, the entity A will have a stale reference item.

+1

Thanks for working on this. Makes sense to clean up, I'd even suggest to have it enabled by default, but then keep it disabled for existing installations with update function.

1. +++ b/tests/src/Kernel/PurgeStaleJobItemsAndJobsTest.php
   @@ -0,0 +1,121 @@
   +
   +  protected $node1;
   +  protected $node2;
   +  protected $jobItem1;
   +  protected $jobItem2;
   +  protected $job;
   

   The properties need documentation.

2. +++ b/tests/src/Kernel/PurgeStaleJobItemsAndJobsTest.php
   @@ -0,0 +1,121 @@
   +  /**
   +   * Test purging of stale TMGMT entities.
   +   *
   +   * Option is disabled.
   +   */
   +  function testPurgeStaleDisabled() {
   

   I think we can write the first line for those two tests so they explain what it does in a single line, something like:

   Tests purging when enabled/disabled.

3. +++ b/tmgmt.module
   @@ -124,6 +126,38 @@ function tmgmt_cron() {
   +    $select = Drupal::database()->select('tmgmt_job', 'tj');
   +    $select->join('tmgmt_job_item', 'tji', 'tji.tjid = tj.tjid');
   +    $select->addField('tj', 'tjid');
   +    $select->condition('tji.item_type', $entity->getEntityTypeId());
   +    $select->condition('tji.item_id', $entity->id());
   

   Pretty sure this can be an entity query instead.

   Instead of loading the jobs, why not load the job items directly and delete them.

   And then I would do another query to look if there are more job items with the same job id, just doing a count query.

   The reason for that is that we have continuous jobs, which might have dozens or hundreds of job items, and getItems() would load them all.

@berdir The earlier patch was not working as 'purge_stale' config was not getting save. Updated it and made changes as per suggestions mentioned by you.

Above patch had malformed. Attaching new one.

@Berdir I think even on the existing site where the module is in use. Stale Jobs remove functionality should be enabled by default.

Tested patch from #13. Works good. Only tests should be fixed
I don't think option should be enabled by default on existing projects. It may become "surprise" for someone. Would advice against using patch from #15

I've adopted UI tests to the changed behavior with deleted entities, and have committed to the MR https://git.drupalcode.org/project/tmgmt/-/merge_requests/35 together with the last patch from @pratik_kamble. Please review.

A re-roll for #19 which I needed after applying a patch from Issue #3097660

Unfortunately this patch is adding code without explicit accessCheck(TRUE/FALSE) to content entity queries.

For example in the function "tmgmt_entity_delete" in the code file "tmgmt.module". This case is really worrying because it causes errors in the removal of any content, leaving Drupal 10 websites completely inoperative.

Added accessCheck() to the MR as it is a blocker when deleting any kind of entity, including configuration, so also causes deployment issues.

We might still decide whether or not we want this behaviour to become the default one (see #16).

Just adding diff from the MR to use with composer. The diff applies for 1.15 modules version, but has conflicts with the latest dev version.

Yes, this conflicts badly with another feature we added.

@berdir
what are the new features that conflicts with this patch?

Rebased with latest 8.x-1.x and fixed conflicts, had to rename tmgmt_update_8011 as tmgmt_update_8012.

If you previously applied one of the previous patch from this issue with tmgmt_update_8011, just make sure to run the one from https://git.drupalcode.org/project/tmgmt/-/blob/8.x-1.x/tmgmt.install?re..., otherwise creating a job will fail with Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'file_count'.

There are now merge conflicts that need to be resolved.

I'll work on this

I rebased and made a few key changes:
- Don't delete continuous jobs when they become empty, they are persistent containers that keep collecting items for new content (addresses the open review note on `tmgmt.module:157`).
- For existing installs, set purge_stale to FALSE, as per #16 (so the feature is opt-in and existing sites keep their current behavior)
For new installs, set purge_stale to TRUE, as per #10
- Skip tmgmt's own entities in the delete hook, so purging job items/jobs doesn't pointlessly re-enter it.
- Updated test coverage
- General tidy-up: guard clauses, return types, and simplifying the kernel test using entity_test.

---
#2913008: Cannot abort or delete item if the source entity has been deleted is a related but separate issue and shouldn't be bundled here IMHO: it addresses the issue when a job item whose source is already gone can't be aborted or deleted from the UI, whereas this issue auto-removes job items when their source is
deleted. Different problems; both still needed, but better addressed separately.

A question on the new settings default value on new installs

The MR doesn't change the behavior of existing sites but applies as default for new installs. This is correct, IMHO. There is test coverage and the code looks good. RTBC!

Thank you all for contribution. Merged