Support alphanumeric entity IDs and config entities

Not sure if there are explicit rules for having numeric IDs for content entities, but I don't think you should be doing that. There could be huge performance issues when using non-numeric and numeric field references in a single database column. MySQL ensures that your indexes are not used if the data types of the columns do not match (https://dev.mysql.com/doc/refman/5.7/en/mysql-indexes.html, search the page for 'Comparison of dissimilar columns').

Besides that a lot of contrib modules are built on the assumption that content entity IDs are numeric (like this module), which means you won't be able to integrate with those modules.

It would be better if drupal switched to the UUIDs to prevent issues like this (same if you want to reference config and content entities through a single field), but that's another discussion I guess. See #2874548: Add a choice to use UUID in entity reference values instead of the entity's ID and #1726734: Replace serial entity IDs with UUIDs in URLs, or even globally?.

We could make the case to switch to UUIDs to also be able to support config entities, but I'm not sure if it would make sense for this module. Especially now we are adding language and revision support.

TL;DR;

• Changing the column type for content entities with non-numeric IDs is not a good idea. Content entities should not do that.
• Changing the column type to switch to UUIDs and support config entities is probably also not a good idea.

The initial approach of this module was to start with something with reduced complexity that would address (hopefully) the 80% use case. That's where the "content-entities-only" restriction appeared.

Now TBH we may already be supporting features that increase that complexity to a point where maybe the restriction doesn't make sense anymore... For instance, if we allow tracking of all languages or revisions that point to a given media entity, why shouldn't sites be able to track in which webform submissions or custom blocks the same entity is being used?

Maybe the restriction should be to FieldableEntityInterface instead of ContentEntityInterface? I'm sure there are some fieldable entities out there we don't want to support, though. In any case, opening the door to these use cases would definitely imply modifying the schema...

Content entities with string ID's are official supported, for example by entity_reference fields, which define their schema dynamically. Yes, some contrib and even core modules don't fully support it, but that is not an argument to not support it, it just means those other modules don't support it, it's not a conflict with them.

Yes, query performance is a problem and also actual compatibility, mysql is one thing but for postgresql, the types must match to be able to do a query. dynamic_entity_reference 8.x-2.x supports that with magic on the database level (triggers and so on), since this is fully behind a service, I think it would be OK to just handle it in PHP and maintain two columns, one for integer, one for string ID's. Could even do things like dynamically defining views integration to join one or the other column, could also easily be a follow-up and as a start just support integer ID's in views.

> Maybe the restriction should be to FieldableEntityInterface instead of ContentEntityInterface? I'm sure there are some fieldable entities out there we don't want to support, though.

There is no difference between FieldableEntityInterface and ContentEntityInterface in regards to their ID. I'm not sure why you wouldn't want to support some entity types as long as it is configurable in general, so we don't end up tracking things like the node type of nodes.

As you said, supporting webform references would IMHO be a great use case. that is a config entity and would definitely need support for string ID's.

Thanks @Berdir for the feedback!

I didn't know entity_reference fields define their target schemas dynamically, hence implicitly supporting string IDs... IMHO that alone is a good argument for supporting it too here.

Maybe then the best compromise could be to have 2 columns, and transparently handle inside the methods that write stuff to the database whether to use one or the other? The only impact this would have in this module's functionalities would be views integration, but as mentioned above, nothing prevents us from letting only int IDs be supported in views in a first step, and adding the magic to support the extra column in a follow-up.

First small step, let's see what this breaks.

Not bad for an initial break-all-the-things... let's try again.

(The ::isInt() helper is obviously not necessary but I feel like it may make things more legible this way?)

Moar breaking refactoring

And this should provide a basic integration with config entities (tested in the patch with Webforms).

If the testbot agrees with me, it may be a good point for testing / feedback before merging.

Oh the test dependency thing...

🤦

You don't need to use test_dependencies, just use require-dev in composer.json, that works immediately, even in patches.

Oh good to know that, thanks! :)

Various comments, didn't review everything also didn't look at the 2.x branch before.

1. +++ b/entity_usage.install
   @@ -75,9 +88,19 @@ function entity_usage_schema() {
        ],
   -    'primary key' => ['target_id', 'target_type', 'source_id', 'source_type', 'source_langcode', 'source_vid', 'method', 'field_name'],
   +    'primary key' => [
   +      'target_id',
   +      'target_type',
   +      'source_id',
   +      'source_type',
   +      'source_langcode',
   +      'source_vid',
   +      'method',
   +      'field_name',
   +    ],
   

   A bit surprised that such a long key is even supported as the primary key. Lets make sure this also works on sqlite/poostgresql.

2. +++ b/entity_usage.install
   @@ -75,9 +88,19 @@ function entity_usage_schema() {
        'indexes' => [
          'target_entity' => ['target_type', 'target_id'],
   +      'target_entity_string' => ['target_type', 'target_id_string'],
          'target_entity_count' => ['target_id', 'target_type', 'count'],
        ],
   

   We should probably check which indexes are actually required and in which order themy make sense.

   For example, you have "target_id, target_type, count" and "target_type, target_id".

   And index doesn't need to use all the keys, but the order is relevant. That means that those two overlap and the one with count should actually also work for a query that only looks for target_id and target type.

   That said, is there really a query that needs to have a condition on the target type *and* count, isn't count just the value that you load then?

   And target_type/target_id should even be covered by the primary key already, wondering if it would be more efficient to have the type first, though.

3. +++ b/entity_usage.module
   @@ -62,10 +49,6 @@ function entity_usage_entity_predelete(EntityInterface $entity) {
     */
    function entity_usage_entity_translation_delete(EntityInterface $translation) {
   -  // Only act on content entities.
   -  if (!($translation instanceof ContentEntityInterface)) {
   -    return;
   -  }
      \Drupal::service('entity_usage.entity_update_manager')->trackUpdateOnDeletion($translation, 'translation');
    }
   

   Hm, tracking non content entities as the *source* is actually a diferent topic. Does that really make sense? Is there any plugin that supports that? config entities don't really have any generic concepts like fields or so, the onl thing I can think of would be to process properties that have a format, but that might be quite tricky to identify.

4. +++ b/entity_usage.views.inc
   @@ -52,6 +52,11 @@ function entity_usage_views_data_alter(array &$data) {
            'relationship' => [
              'base' => 'entity_usage',
   +          // @todo For now our views integration only supports entities with
   +          // integer IDs. We could make this smarter in the future by using
   +          // the correct base field for the relationship depending on the column
   +          // that this entity will be using in our table (i.e. either target_id
   +          // or target_id_string).
              'base field' => 'target_id',
              'field' => $entity_type->getKey('id'),
   

   You should then also check if that id field really is an integer then and skip the others. Otherwise users might try to add it and then they get errors.

5. +++ b/src/Controller/ListUsageController.php
   @@ -98,38 +94,46 @@ class ListUsageController extends ControllerBase {
            foreach ($all_usages as $source_type => $source_ids) {
              foreach ($source_ids as $source_id => $entity_usages) {
   -            /** @var \Drupal\Core\Entity\ContentEntityInterface $source_entity */
   +            /** @var \Drupal\Core\Entity\EntityInterface $source_entity */
                $source_entity = $this->entityTypeManager->getStorage($source_type)->load($source_id);
   +            // Skip early if this entity for some reason doesn't exist anymore.
   +            if (!$source_entity) {
   +              continue;
   

   Idea, maybe for a separate issue: Since we already have an outer loop of type/ids, why not do a single loadMultiple()? If you then loop over that you only loop over existing ids, can get the count with $source_ids[$entity->id()] and automatically ignore any garbage data

6. +++ b/src/EntityUpdateManager.php
   @@ -152,18 +165,18 @@ class EntityUpdateManager {
   +  protected function allowSourceEntityTracking(EntityInterface $entity) {
   +    $allow_tracking = FALSE;
   +    $entity_type = $entity->getEntityType();
        $enabled_source_entity_types = $this->config->get('track_enabled_source_entity_types');
   

   I didn't look at the 2.x branch yet, maybe this is handled somewhere else?

   Wouldn't it make more sense to limit by target entity type, not source? If I want to track media usage, I don't care what is using it, I want everything to be counted, nodes, paragraphs, terms, blocks.. why would I want to not count some of that?

7. +++ b/src/EntityUsage.php
   @@ -272,4 +295,18 @@ class EntityUsage implements EntityUsageInterface {
   +  private function isInt($value) {
   +    return ((string) (int) $value === (string) $value);
   +  }
   

   I think private should only be used for methods/properties in extremely rare situations, if I want to override the service and change one method then I couldn't call this.

   I think it is more common to look at the field definition of the entity type to decide that, see \Drupal\Core\Entity\ContentEntityStorageBase::cleanIds(). Of course that only works for content entities..

8. +++ b/src/Form/BatchUpdateForm.php
   @@ -86,9 +85,19 @@ class BatchUpdateForm extends FormBase {
   +      if (!is_array($to_track) && ($entity_type->entityClassImplements('\Drupal\Core\Entity\ContentEntityInterface'))) {
   +        // When no settings are defined, track all content entities by default,
   +        // except for Files and Users.
   +        if (!in_array($entity_type_id, ['file', 'user'])) {
   +          $track_this_entity_type = TRUE;
   +        }
   +      }
   

   similar as above, in regards to source/target tracking, what exactly are we now checking here?

   If I have a media reference on a user entity (profile image), why shouldn't that be tracked? Isn't it references *to* users/files that I'd want to exclude?

9. +++ b/src/Form/EntityUsageSettingsForm.php
   @@ -131,14 +135,15 @@ class EntityUsageSettingsForm extends ConfigFormBase {
    
        // Entity types (target).
        $form['track_enabled_target_entity_types'] = [
          '#type' => 'details',
   -      '#open' => TRUE,
   +      '#open' => FALSE,
          '#title' => $this->t('Enabled target entity types'),
   

   ah, so there is a target type setting too. still confused why users are excluded from the source by default :)

@Berdir thanks for the feedback!

1) I triggered the test on sqlite and postresql and it passed in the latter and had one failure in the former. I'd like to investigate the failure a bit further, but I'm not sure it's due to an incompatibility at this level, once other tests passed there?
In any case, do you think having an extra column (incremental) just for a record id would make this more robust?

2) Agreed, tried to simplify it a bit.

3) I agree that right now there is no real use case for tracking config entities as sources. But the reasoning here was: Once
- both source and target entity types are configurable,
- only content entities are enabled by default
- the DB needs to have 2 columns anyway in order to support content entities with string IDs,
opening the door to accept config entities as sources too didn't have any negative side effect except for "polluting" a bit more the config form with some (for now) useless options. But who knows what crazy plugins people will want to implement in the future... so we are already prepared for that.
No big deal though, if we think the UI is too packed with confusing options, we can easily show on the config form only content (or fieldable) entities, without needing to explicit "prohibit" them inside the methods logic, IMHO.

4) done, thanks!

5) Good idea, but let's do that in #2952210-7: Improve list usage page, where we'll probably refactor that code. Left a comment there so we don't miss this.

6) IMHO It does make sense to limit by source too. For example, imagine you have a site with hundreds of thousands of nodes and millions of comments, and for some business reason, you only need to track your custom entity type that uses media entities. You save a lot by just leaving your custom entity enabled as source.

7) done, thanks!

8) Same as 6 ? (please indicate if not)

9) Yeah, excluding files and users from the default values was kind of a performance reason too. I think in the 80% use case the tracking feature will be probably used for "standard" content entities (nodes, media, comments, paragraphs, etc.). Users can always enable an entity they specifically need, and run the batch update again.

And now with a new plugin supporting block_field field types tracking views config entities as well.

(edited, the comment was for a different issue... too many tabs open 🤦 )

Marking this as fixed for now, if we find some issues with this refactor let's fix them in follow-ups.

Also, I've left a note in the project page that for now this has only been tested with MySQL.

The changes to entity_usage_schema are not supported with an upgrade path. Perhaps repeat entity_usage_8201() ?

The existing update hook should take care of the update between 1.x and 2.x, no? Or you mean between the initial 2.x version and after the commit in #24 ? If so, I don't think we should worry too much, sites in production shouldn't have been using the 2.x at its early stages, and this was committed before the 2.x was marked as visible / recommended.
Or maybe I'm missing what you are referring to, please indicate if so.

The project I am working on already ran 8201. I have ran it again from a custom module update hook. I agree the existing update hooks should be sufficient for existing projects, so all is fine for now.