Problem/Motivation

Currently, the module completely disregards revisions by only caring about the last-saved revision of each entity. In environments where revisions are used to be able to roll-back to a previous state, or where moderation workflows with roll-back are being used, this doesn't make sense and could lead to wrong information.

Proposed resolution

- Decide the best approach and expected behavior for each use case
- Implement it, with test coverage
- Commit

Hopefuly this task will solve or make obsolete the following issues:
- #2826906: Get entity usage count for node revision and connect the entity to it
- #2821411: Add support for Revisions and Content Moderation
- #2844494: Add support for paragraphs and entity_reference_revisions field types

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#7 2949853-6-1.x-full.patch177.69 KBseanb
#7 2949853-6.patch1.34 KBseanb
#5 2949853-5-2.x-do-not-test.patch69.54 KBseanb
#5 2949853-5-1.x-full.patch177.53 KBseanb
#3 2949853-3-2.x-do-not-test.patch69.3 KBseanb
#3 2949853-3-1.x-full.patch177.53 KBseanb

Comments

marcoscano created an issue. See original summary.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

Some more background about the current approach and ideas to build on:

Part of the reason we weren't specifically dealing with revisions so far was not to assume anything about the use case. For example, sites could want to use the module to prevent "used" entities from being deleted (let's call this the "Integrity Use-Case"). Other sites, however, could use the module to grant access to a child entity if the visitor has access to the parent entity (let's call this the "Access control Use-Case").

While for integrity (if the site enables revisioning and wishes to be able to roll-back) we most likely want to take into account all revisions in order to ensure integrity in all of them, for access control we are really only interested in the default (last published) revision. In fact, in the latter scenario tracking all past revisions would be a problem that could also lead to inaccurate behavior.

I'm not sure if there could be some sort of middle-ground that could be used in both scenarios without adding too much complexity.

At this point, however, I'm leaning towards primarily supporting the integrity use case, trying not to be in the way of the access control use case (maybe by adding some helpers that would check usage only on the default revision? I wonder if that would be feasible in sites with lots of revisions, though...). Also, I feel that access control use cases already need more custom code and need to deal with more complexity, caches, etc, so maybe it's not a big of a deal if this module, out-of-the-box, prioritizes sites looking for integrity usage cases.

Thoughts?

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Status: Active » Needs review
StatusFileSize
new 2949853-3-1.x-full.patch177.53 KB
new 2949853-3-2.x-do-not-test.patch69.3 KB

Here is a first patch just to get a "working" version. There s still an issue when deleting a revision in a multilanguage environment. Deleting a revision seems to delete it in all languages. The references are only deleted for the current language. This leaves some records that can't be loaded anymore.

Besides that, it seems to record everything as expected. The list is going to be huge for sites with lots of changes and references. Not sure how to fix that, maybe just add a pager? We could also create more levels, like only showing the highest vid per entity and add a link to the overview to see all revision with references for that node.

Another thing I have doubts about is the count column. Now that we store every unique reference as a separate record, we could probably skip that?

The last submitted patch, 3: 2949853-3-1.x-full.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
StatusFileSize
new 2949853-5-1.x-full.patch177.53 KB
new 2949853-5-2.x-do-not-test.patch69.54 KB

New try to fix the test. Had some trouble creating the interdiff, but since there are no review yet this is hopefully ok.

The last submitted patch, 5: 2949853-5-1.x-full.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
StatusFileSize
new 2949853-6.patch1.34 KB
new 2949853-6-1.x-full.patch177.69 KB

I guess there was more to fix. Committed the progress so far to the 2.x branch.

Created #2952187: Entity usage for revisions not correctly removed (2.x), the usage tab currently contains a extra check to make sure the page does not break when a revision is no longer there.
Created #2952210: Improve list usage page To improve the usage tab.

If we want to remove the count column that could also be a separate issue.

  • seanB committed 0bc7906 on 8.x-2.x 
    Issue #2949853 by seanB: Support revisions / moderation workflows
  • seanB committed 69cd3bc on 8.x-2.x
    Issue #2949853 by seanB: Support revisions / moderation workflows (small...
marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Version: 8.x-1.x-dev » 8.x-2.x-dev
marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Needs review » Fixed

While reviewing this / testing I've found #2952448: Entity being tracked twice if inside paragraphs.

Let's just do all remaining work of testing / improving this in follow-ups, this is already committed so the easiest thing to do is just to move on.

Thanks @seanB!

Anonymous’s picture

Anonymous (not verified) commented

Hey,

Great work on this module and the 2.x branch!

Just one remark about this new functionality though: is it possible to make the tracking of entities in revisions optional? Right now it is enabled by default, which in my use case doesn't make sense; I don't want usage in revisions to clutter up the usage list page.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

Yeah we are indeed considering that. Probably the solution will be to have 2 tabs in the usage list page: one (default) only for the default revisions, and another one if people want to get a list of all revisions tracked. This will probably happen in #2952210: Improve list usage page

Anonymous’s picture

Anonymous (not verified) commented

The two tabs solution will still track everything and would leave you with a lot of unused data in the db though. For larger sites with a lot of entities that have revisions enabled, this could lead to a huge entity_usage table. So I would prefer the option of the tracking being optional to begin with.

I've discovered hook_entity_usage_block_tracking() so that allows me to block tracking of revisions.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

berramou’s picture

berramou commented

Hello @sam.spinoy@gmail.com can you please share your code how did you block tracking revisions ?

flyke’s picture

flyke commented

@berramou,
(and others looking for the same answer: do not track revisions)

see: https://www.drupal.org/project/entity_usage/issues/3056562

/**
 * Implements hook_entity_usage_block_tracking().
 */
function hook_entity_usage_block_tracking($target_id, $target_type, $source_id, $source_type, $source_langcode, $source_vid, $method, $field_name, $count) {
  if (!empty($source_type) && !empty($source_vid)) {
    $entity_revision =\Drupal::entityTypeManager()
      ->getStorage($source_type)
      ->loadRevision($source_vid);
    if (!$entity_revision->isDefaultRevision()) {
      // To Block a revision entity from tracking return TRUE.
      return TRUE;
    }
    return FALSE;
  }
  return FALSE;
}
java008’s picture

java008 commented

How to check the condition entity is getting created or deleted inside this hook?