Contrib.social

Redo CommentStatisticsInterface

Open on Drupal.org →
Created on 11 August 2014, over 10 years ago
Updated 30 January 2023, almost 2 years ago

Two things requested:

  1. Make read() return a traversable object instead of an array (better resource usage. See below for story.)
  2. Change arguments of crud functions as follows:
    create($entity, $fields(config objects))  -> ($entity, $field_names*)
update($comment)                          -> ($entity, $field_names*)
read($entities, $entity_type, $accurate*) -> ($entity_type, $entities, $field_names*, $accurate*)
delete($entity)                           -> ($entity, $field_names*)
larowlan #8: add deleteMultiple($entity_type, $entity_ids, $field_names*)

(* = optional. $entity means 'commented/parent entity'.)

We can still clean up the interface now because it's fresh enough that noone 'external' is using it yet.

Beta phase evaluation

Justification

Constraints imposed by practical usage

  • read() is the most important method in this service and should keep an array as the first argument for speed. (Each time I look at this I want to change ($entities, $entity_type) to ($entity_ids, $entity_type) to make things less redundant... but then I see the definition of hook_entity_storage_load(). So I have no opinion on that.)
  • CommentStatistics::update() inserts a record if one does not exist. That means we cannot, at the moment, delete statistics records for an entity before deleting its comments; that would recreate the record.

Detailed changes

  • read - adding $field_names: it's quite possible that someone making a website with multiple comment fields, wants to read (maybe a lot of) statistics for only one of the fields. Also: DX/the presence of this parameter implicitly reminds you that the return value can hold multiple records for the same entity. (Forgetting this led to my mess-up re #1.)
  • delete/update - adding $feld_names: it's possible that you only want to delete records for one field name as well. Adding it to update() isn't that important, but: adds consistency (DX).
  • create - replacing the field-definition-config objects by their machine names: for unification with other methods, and because it's highly unlikely any service implementation will need the objects. (The current code does not use them.)
  • create - making the field_names optional: because we can; because of consistency with other methods and we don't want to make it required on all the methods, that would be a pain.
  • update - changing to ($entity, $field_names): because of consistency; from a code perspective there is no reason why this method should have different arguments than create(); also, having $comment falsely gives the impression that you should call this method for each comment individually, if you do mass operations (e.g. delete) on comments attached to the same entity.
  • delete these comments are superseded by larowlan's #8 suggestion; ...I'm not sure about this. It seems like
    • we will at some point want to be able to delete all the records for an entity_type + field_name combination, i.e. for all entities. This patch doesn't actually implement that; it just makes it so you could implement something special like e.g. array(0) to mean "all entities".
    • loading full entities in order to delete related data, seems a bit strange. (In comment_field_instance_config_delete(), we would not have the entities yet, only the IDs from an EntityQuery.)

    But if anyone wants to keep the method signature to ($entity, $field_names), I'll change that back.

Patch details

  • The change in update() adds a 10-line code block + a foreach that starts with a 3-line if{}, near the top. The rest of the code didn't change, it was just indented.
  • The test: I found working code that mocked 'iterator methods' on http://stackoverflow.com/questions/15907249/mocking-an-iterator-class-wi..., and not inside the D8 source code, so copied it. Without changing, so it's 'too complete' now. I have no idea if that can be done better, so have at it if you want.
  • CommentStatisticsInterface::read() adds back some comments deleted from CommentStorageInterface::updateEntityStatistics() by #2280861: CommentStatistics service followup → .

Remaining tasks

Make CommentStatistics language aware, as noted around #2428795-94: Translatable entity 'changed' timestamps are not working at all → . This should be done in a new issue.

Original issue text / point 1

Earlier, I filed an issue #2259209: Fix CommentStatistics::read() → because I wanted read() to return an array like the interface documentation said it does.

Well, that was kinda silly. Since I was keying the array by entity_id, it led to the bug which is solved by #2292115: Comment statistics is no correctly loaded into entity object if more than 1 comments field → .

Unfortunately I did not see that last issue until it was fixed. Because

  • returning a non-keyed array has no real advantage over returning an iterator
  • returning an array takes more processing power and memory (it's conceivable that you want to get a lot of statistics data

...so I'd like to restore the situation to before these two issues were applied :-p ...except the interface documentation should be amended. Right now, we can still be sure we can change the interface without penalty.

✨ Feature request
Status

Needs work

Version

10.1 ✨

Component
Comment  →

Last updated 11 days ago

Created by

🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

Live updates comments and jobs are added and updated live.
  • API clean-up

    Refactors an existing API or subsystem for consistency, performance, modularization, flexibility, third-party integration, etc. May imply an API change. Frequently used during the Code Slush phase of the release cycle.

  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment almost 2 years ago →
    needs-review-queue-bot

    The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024