Problem/Motivation

Our API currently has 2 methods for modifying DB records:

\Drupal\entity_usage\EntityUsageInterface::add()
\Drupal\entity_usage\EntityUsageInterface::delete()

They are responsible for incrementing / decrementing the count of a given row in the DB, creating a new row if none exists, or removing it if the count goes to 0.

Also, they trigger the USAGE_ADD and USAGE_DELETE events after doing that.

Now that we are adding new columns to the table (source_vid, source_language, field_name), the count column changes a bit its meaning, because it will only be different than 1 in cases where the same entity/revision/language is pointing to the same target entity in the same field. Because of that I believe we should keep the count column, but I see little value added to having two different methods for updating the records.

Also, the ::delete() method, strictly speaking, is not triggering the event correctly... IMHO the actions "decrease usage count, but still leaving a non-zero count" is not the same action as "remove the row", so at least the current implementation should maybe trigger two different events for that?

While at it, we could look into other points for simplification / improvement, DX-wise.

Proposed resolution

1) Remove the ::add() and ::delete() methods, in favor of a single

\Drupal\entity_usage\EntityUsageInterface::registerUsage()

This method would be responsible for:
- Creating a new row if none exists for a given combination of: source_id, source_type, source_vid, source_language, target_id, target_type, field_name
- Updating the existing row with the received count
- Deleting the existing row if the received count is <= 0

2) Create new helper methods for deleting records:

- \Drupal\entity_usage\EntityUsageInterface::deleteByField()
- \Drupal\entity_usage\EntityUsageInterface::deleteBySourceEntity()
- \Drupal\entity_usage\EntityUsageInterface::deleteByTargetEntity()

3) Move "remove" logic away from plugins

Now that new helper methods for removing records are available, we don't even need to call the plugins when an entity is being deleted. EntityUpdateManager::trackUpdateOnDeletion() now handles all logic necessary to update the DB records when an entity is deleted, and plugins no longer need to implement ::trackOnEntityDeletion()

4) The hook hook_entity_usage_block_tracking() no longer receives an "action" parameter

Once now all operations adding/removing records in the DB are being done through the same registerUsage() method.

5) Events are now more specific

After this change, the events triggered by this module while operating on the DB are:
- USAGE_REGISTER
- DELETE_BY_FIELD
- DELETE_BY_SOURCE_ENTITY
- DELETE_BY_TARGET_ENTITY
- BULK_DELETE_SOURCES
- BULK_DELETE_DESTINATIONS

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 2951751-16.patch44.13 KBmarcoscano
#15 interdiff-13-15.txt6.98 KBmarcoscano
#15 2951751-15.patch48.69 KBmarcoscano
#13 interdiff-10-13.txt8.14 KBmarcoscano
#13 2951751-13.patch49.29 KBmarcoscano
#10 interdiff-9-10.txt9.74 KBmarcoscano
#10 2951751-10.patch43.96 KBmarcoscano
#9 interdiff-7-9.txt4.46 KBmarcoscano
#9 2951751-9.patch37.91 KBmarcoscano
#7 interdiff-5-7.txt1.3 KBmarcoscano
#7 2951751-7.patch34.37 KBmarcoscano
#5 2951751-5.patch33.63 KBmarcoscano

Comments

marcoscano created an issue. See original summary.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Priority: Normal » Minor

Started working on this and was quite far on refactoring it, but then realized that there is some benefit in having the ::delete() method separate.

Once the ::delete() method has many more optional parameters, this allows for a sort of "bulk" deletion of rows with a single query. This is being used, for example, when an entity is deleted and we want to remove all rows where this entity is a target.

Maybe this alone can justify leaving both methods as they are today... not sure.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented

You could always add a separate method with much less parameters for bulk deletes? Or a bunch of methods like deleteByEntityType, deleteByTargetEntity, deleteByField (which we are missing now btw, if you delete a field the tracking is not updated).

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: Active » Needs review
StatusFileSize
new 2951751-5.patch33.63 KB

OK let's see what this breaks.

Status: Needs review » Needs work

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

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Needs work » Needs review
StatusFileSize
new 2951751-7.patch34.37 KB
new interdiff-5-7.txt1.3 KB

more breaking stuff

Status: Needs review » Needs work

The last submitted patch, 7: 2951751-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Needs work » Needs review
StatusFileSize
new 2951751-9.patch37.91 KB
new interdiff-7-9.txt4.46 KB

Commented out some parts of the failing kernel test because probably we need to refactor the test itself. I don't see how the functional tests could pass if the API methods weren't working.

In any case, the tests will all receive some love in #2952008: Automated tests: Refactor existing and add missing coverage

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
StatusFileSize
new 2951751-10.patch43.96 KB
new interdiff-9-10.txt9.74 KB

This would simplify things even further, now that the API allows that.

Thoughts?

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented

Just a quick scan of the code. Still need to test but just posting this already. Seems like this would improve the DX significantly. I'm not sure if the count can ever be more than 1 (or if that is even relevant now we track very specifically?). Can we use a delete bool instead of a count? If not a delete it's always an add?

  1. +++ b/src/EntityUpdateManager.php
@@ -86,22 +86,41 @@ class EntityUpdateManager {
+      case 'default':

    I think this line can be removed. 

  2. +++ b/src/EntityUsageInterface.php
@@ -10,10 +10,17 @@ use Drupal\Core\Entity\EntityInterface;
+   * If called with $count >= 1, the record matching the other parameters will
+   * be updated (or created if it doesn't exist). If called with $count == 0,
+   * the record will be deleted.

    This looks kind of tricky. Can we pass an explicit delete bool or something? 

  3. +++ b/src/EntityUsageInterface.php
@@ -35,56 +42,58 @@ interface EntityUsageInterface {
+  public function bulkDeleteTargets($target_type);
...
+  public function bulkDeleteSources($source_type);
...
+  public function deleteByField($source_type, $field_name);
...
+  public function deleteBySourceEntity($source_id, $source_type, $source_langcode = NULL, $source_vid = NULL);

    Really like the new delete methods!

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

@seanB thanks for having a look!

IMO even if it is not possible right now, in the future it could be possible that the count column had different values. For example, if a given set of source_id/source_type/source_vid/source_langcode points to a given target in the same field more than once. For example, in a WYSIWYG where several links point to the same entity, or the same entity was embedded more than once. Right now our logic is tracking only usage for those, but we may improve that at some point in the future, so the API would already support that.

In those scenarios, identical calls but with different non-zero $count values are legitimate, meaning we only want to update the record row, instead of inserting a new one.

I'm not really against adding a specific flag for the deletion, but given the above, do we still think it will improve things? For example, the delete calls would then be:

$this->usageService->registerUsage($target_id, $target_type, $source_entity->id(), $source_entity->getEntityTypeId(), $source_entity->language()->getId(), $source_entity->getRevisionId(), $this->pluginId, $field_name, NULL, TRUE);

instead of:

$this->usageService->registerUsage($target_id, $target_type, $source_entity->id(), $source_entity->getEntityTypeId(), $source_entity->language()->getId(), $source_entity->getRevisionId(), $this->pluginId, $field_name, 0);

I kind of still think the second is clearer.. At that point maybe we could just simply create a new method ::deleteRecord() to just delete a specific row? Maybe this is the simplest after all...

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Priority: Minor » Normal
StatusFileSize
new 2951751-13.patch49.29 KB
new interdiff-10-13.txt8.14 KB

This adds a new ::deleteRecord() helper, that should make things a bit easier to use.

Thanks!

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented

As far as I can see everything seems to work fine! The code seems a lot simpler now, there are lot's of nice improvements in the patch. Can you update the IS with a list of improvements?

About the original point of the issue to unify the add/delete method. Having a separate delete method seems to go against the point of the issue. My biggest concern is/was the DX. The fact that a count of 0 actually deletes a DB records feels kind of wrong, but I guess it is better than keeping the record with a count of 0. And when the count is 0 for a field, I guess the only correct thing to do is not record the usage/delete an existing record. So let's go with #10 after all.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
StatusFileSize
new 2951751-15.patch48.69 KB
new interdiff-13-15.txt6.98 KB

@seanB thanks for reviewing!

Yep, I feel the same, thanks! So I reverted to the approach in #10 here but kept one small improvement in the switch statement.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
StatusFileSize
new 2951751-16.patch44.13 KB

Re-rolled because while reverting some of the changes in the previous patch I kind of messed up with the diff.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Title: Simplify add/remove methods in favor of a single "register()" method » Simplify API methods and create new helpers
Issue summary: View changes

Updating IS

  • marcoscano committed c665a8a on 8.x-2.x 
    Issue #2951751 by marcoscano, seanB: Simplify API methods and create new...
marcoscano’s picture

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

Done. Thanks all!

Status: Fixed » Closed (fixed)

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