Change terminology for module

Is 'destination' the best solution? I think 'target' fits better (target_id).

Yeah, I kinda like source/target better too, but don't really feel strong about it, as long as we use whatever we choose consistently everywhere.

@seanB amazing work! Thanks a lot!!

Once I hadn't reviewed the fieldname modifs, I've gone over the full patch.

1. +++ b/entity_usage.install
   @@ -12,52 +12,59 @@ function entity_usage_schema() {
   +        'not null' => FALSE,
   

   I would enforce this to be not null. Any scenario where it could not be the case?

2. +++ b/entity_usage.install
   @@ -72,3 +79,13 @@ function entity_usage_update_8001(&$sandbox) {
   +
   +/**
   + * Recreate the entity usage table with the new schema.
   + */
   +function entity_usage_update_8002() {
   +  $schema = \Drupal::database()->schema();
   +  $schema->dropTable('entity_usage');
   +  $new_table_schema = entity_usage_schema();
   +  $schema->createTable('entity_usage', $new_table_schema['entity_usage']);
   +}
   

   Can we rename this hook to 8201?

   Also, even if we are not offering BC, sites with existing data shouldn't lose the statistics they already have. Once our data can always be recalculated, I believe the easiest thing to do here is to just trigger a bulk update after re-creating the table with the new schema.

3. +++ b/src/Controller/ListUsageController.php
   @@ -55,49 +61,57 @@ class ListUsageController extends ControllerBase {
   +   * @throws \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
   

   Thanks! :)

4. +++ b/src/EntityUsage.php
   @@ -56,38 +55,39 @@ class EntityUsage implements EntityUsageInterface {
      /**
       * {@inheritdoc}
       */
   -  public function add($t_id, $t_type, $re_id, $re_type, $method = 'entity_reference', $count = 1) {
   -
   +  public function add($target_id, $target_type, $source_id, $source_type, $method = 'entity_reference', $field_name = NULL, $count = 1) {
   

   In the origin of times, entity_reference was the only tracking method available, so it kind of made sense to call the method without this parameter. Nowadays I wonder if we shouldn't just make it required with no default value.

5. +++ b/src/EntityUsage.php
   @@ -56,38 +55,39 @@ class EntityUsage implements EntityUsageInterface {
      /**
       * {@inheritdoc}
       */
   -  public function delete($t_id, $t_type, $re_id = NULL, $re_type = NULL, $count = 1) {
   -
   -    // Delete rows that have an exact or less value to prevent empty rows.
   +  public function delete($target_id, $target_type, $source_id = NULL, $source_type = NULL, $field_name = NULL, $count = 1) {
   

   I'm thinking about the fact that by adding the field name to the schema, now all rows will be unique except for the amount of "usages" in the same field.

   Regardless if we track all usages of the same entity inside the same field or not (we probably want to improve that in the future), maybe it could make sense to simplify the API and replace both add() and delete() methods by a single register() method that would always write the count to the row, and delete the row if called with count 0?

6. +++ b/src/EntityUsage.php
   @@ -97,101 +97,103 @@ class EntityUsage implements EntityUsageInterface {
   -    $event = new EntityUsageEvent($t_id, $t_type, $re_id, $re_type, NULL, $count);
   -    $this->eventDispatcher->dispatch(Events::USAGE_DELETE, $event);
    
   +    $event = new EntityUsageEvent($target_id, $target_type, $source_id, $source_type, NULL, NULL, $count);
   +    $this->eventDispatcher->dispatch(Events::USAGE_DELETE, $event);
   

   If we do the above unifying the method to write to the database, we should also change the events to something like USAGE_TRACK (when registering a count > 0) and USAGE_DELETE when registering a count = 0 (deleting a row).

7. +++ b/src/EntityUsage.php
   @@ -97,101 +97,103 @@ class EntityUsage implements EntityUsageInterface {
   +  public function listUsage(EntityInterface $target_entity, $include_method = FALSE) {
   ...
   +  public function listReferencedEntities(EntityInterface $source_entity) {
   

   Originally the listUsage() was named like this to make an analogy to file usage in core, that is called the same.

   At this point I wonder though if it wouldn't make more sense to call these two methods listSources($target_entity) and listTargets($source_entity... Thoughts?

8. +++ b/src/EntityUsage.php
   @@ -97,101 +97,103 @@ class EntityUsage implements EntityUsageInterface {
   +      $field_name = $usage->field_name ?: '_unknown';
   

   If the schema for this is not null, in what circumstances this could be empty? Maybe it's good to let it break here, once it would mean something is wrong?

9. +++ b/src/EntityUsage.php
   @@ -97,101 +97,103 @@ class EntityUsage implements EntityUsageInterface {
   +        if (!empty($references[$usage->source_type][$usage->source_id][$field_name])) {
   +          $count += $references[$usage->source_type][$usage->source_id][$field_name];
   

   Yeah, this definitely will get hairy as we start adding more info to the array... :) Not sure though an easy way to simplify and also have a single method capable of getting all information. But I think we can leave this improvement for a later iteration in any case.

10. +++ b/src/EntityUsageInterface.php
    @@ -10,77 +10,87 @@ use Drupal\Core\Entity\EntityInterface;
    +   *   (optional) The method the target entities is being referenced.
    

    Maybe a better description for this could be

    The method used to relate source with target.
     Normally the plugin id.

    ?

11. +++ b/src/Events/EntityUsageEvent.php
    @@ -97,32 +107,32 @@ class EntityUsageEvent extends Event {
    -   * Sets the referencing method.
    +   * Sets the source method.
    

    In this particular case I believe the replacement is not ideal... Maybe The method used to relate source with target would result clearer?

12. +++ b/src/Events/EntityUsageEvent.php
    @@ -97,32 +107,32 @@ class EntityUsageEvent extends Event {
    -   *   The referencing method.
    +   *   The source method.
    

    idem

13. +++ b/src/Events/EntityUsageEvent.php
    @@ -97,32 +107,32 @@ class EntityUsageEvent extends Event {
    -  public function setReferencingMethod($method) {
    +  public function setSourceMethod($method) {
    

    maybe just setMethod directly?

14. +++ b/src/Events/EntityUsageEvent.php
    @@ -157,35 +167,45 @@ class EntityUsageEvent extends Event {
    -   * Gets the referencing method.
    +   * Gets the source method.
        *
        * @return null|string
    -   *   The referencing method or NULL.
    +   *   The source method or NULL.
        */
    -  public function getReferencingMethod() {
    +  public function getSourceMethod() {
    

    same here

15. +++ b/src/Events/EntityUsageEvent.php
    @@ -157,35 +167,45 @@ class EntityUsageEvent extends Event {
    +   * @return null|string
    +   *   The source field name or NULL.
    

    Again, if we set the schema to not null we should update this.

16. +++ b/tests/src/Kernel/EntityUsageTest.php
    @@ -358,9 +370,10 @@ class EntityUsageTest extends EntityKernelTestBase {
    +      'method' => $event->getSourceMethod(),
    
    @@ -378,9 +391,10 @@ class EntityUsageTest extends EntityKernelTestBase {
    +      'method' => $event->getSourceMethod(),
    
    @@ -394,33 +408,35 @@ class EntityUsageTest extends EntityKernelTestBase {
    +      'method' => $event->getSourceMethod(),
    ...
    +      'method' => $event->getSourceMethod(),
    

    if we changed above we'll need to update here too.

That's a weird failure, sometimes just re-triggering the test on the testbot makes it green :/

In my local it passes as is, but let's see if these apparently unrelated changes make any change with the bot.

Thanks!

+1 to move on with the next steps, given no new relevant test failures appear (it shouldn't).

For the record, things I think we should keep track for doing in follow-ups:
- Trigger the bulk-update after the update hook has recreated the table
- Unify the add/remove methods (and adjusting the events)
- Try to simplify the array-of-doom being returned from ::listSources()

Not sure what's wrong with the bot... it's running all core tests instead of ours?

Follow-ups created and added to the roadmap.

Thanks!