🇩🇪Germany @h1nds1ght

This change introduced an issue if you have filenames which are equal to 0 (i.e. 0.jpg), because the while condition does not apply:

while (array_shift($file_parts)) {

I appended a patch to adjust this condition and fix the issue in my case.

Yes I think so, because the topic here is that EntityReferenceRevisionsFieldItemList::referencedEntities loads referenced entities using single entity loads, which still happens via $item->entity.

  public function referencedEntities() {
    $target_entities = [];
    foreach ($this->list as $delta => $item) {
      if ($item->entity) {
        $target_entities[$delta] = $item->entity;
      }
    }
    return $target_entities;
  }

EntityReferenceFieldItemList::referencedEntities i.e. uses loadMultiple...

Wanted to share my d10 patch

@rgpublic For me it works as described with d10.1, thank you!

Do you think it would be possible to integrate some settings like in the current module?

https://git.drupalcode.org/project/ckwordcount/-/blob/3.x/src/Plugin/CKE...

Mostly I miss the "hard limit" option. I think it should be possible to integrate that:

https://ckeditor.com/docs/ckeditor5/latest/features/word-count.html#demo...

I am facing the same issue as described in #163 using embed 1.7 and dev version (ac6f2fe1). For some reason getSelectedElement is null and therefore the succeeding code fails.

                e.ui.componentFactory.add("editEmbeddedEntity", (i=>{
                    const r = new a.ButtonView(i)
                      , n = e.model.document.selection.getSelectedElement()
                      , o = n.getAttribute("drupalEntityEntityUuid")
                      , s = n.getAttribute("drupalEntityEntityType")
                      , l = Drupal.url(`entity-embed/edit-embedded/${s}/${o}`);

I tried to solve the issues mentioned in #54, #55, #56 based on the dropzonejs patch in #52.

Basically I did the following:

- Removed the "Save metadata" button
- Execute "processMetadata" on change
- Prevent pressing Enter on alt + title
- Add class "form-required" if alt is set to required

Not sure if I get your point.

$ids is a mapping of revision_id => entity_id. The loadMultiple call should therefore load each entity in its current revision. The corresponding loaded revision_id of the loaded entity is removed from $ids and the entity aka current revision is added to $entities.

+      foreach ($storage->loadMultiple($ids) as $entity) {
+        if ($entity instanceof RevisionableInterface && in_array($entity->getRevisionId(), $revision_ids)) {
+          unset($ids[$entity->getRevisionId()]);
+          $entities[$entity->getRevisionId()] = $entity;
+        }
+      }

$ids is then used one line after that again:

+      $entities += $storage instanceof RevisionableStorageInterface ? $storage->loadMultipleRevisions(array_flip($ids)) : [];

and loads the remaining revisions, which haven't been loaded in the first step by loadMultiple (the array_flip passes the remaining revision ids). This should be in most cases an empty array, except if the current revision of an loaded entity does not equal an referenced revision. The call for loadMultipleRevisions should then deliver an array with revisions keyed by revision_id, and the result is then added to $entities, which should add the missing revisions since it is also an array of revisions keyed by revision_id.

Finally the collected $entities (aka revisions keyed by revision_id) is used for the field list, with a fallback to the default loadRevision (= current solution).

+      foreach ($revision_ids as $delta => $revision_id) {
+        $entity = $entities[$revision_id] ?? $storage->loadRevision($revision_id);

As mentioned this last loadRevision seems somehow unnecessary.

Maybe this can be written more reasonable, but I can't see what should be wrong with that approach.

Can't see why. Note that i renamed the variable $ids to $revision_ids in the first loop. $revision_ids now holds the delta => revision_id relationship for the field list and $ids is a mapping of revision_id => entity_id, which is only used as temporary variable for loadMultiple and loadMultipleRevisions.

If I remove entries from $revision_ids they are also removed from the final field list.

OK, created another patch. The problem is a little bit that loadMultiple has entity_ids as keys and we cannot rely on that entity_id = revision_id (the last patch did so). This makes it somewhat uncomfortable but one can resolve that with another loop. I checked for revision_ids now, and not for isDefaultRevision, since I think we are interested in revisions here and each revision_id should be unique per storage and should belong to a specific entity_id. But maybe this assumption is wrong. If so we need to add the check for isDefaultRevision.

Also I am not sure if the checks for RevisionableInterface and RevisionableStorageInterface are really necessary. If we can forget about RevisionableStorageInterface we can also remove the last loadRevision.

One thing that comes in my mind are deleted revisions. With that approach a deleted revision might involve 3 db queries.

loadRevisionMultiple() should only lead to a marginal performance improvement, it just does grouped queries but it does not use a static or persistent cache.
Also, I recommend to _not_ use this method at all, at least not until it's optimized. just loop over the items, that will be faster assuming you are viewing mostly default revisions, it will do single loads but from cache.

Well loadRevision executes loadMultipleRevisions, so they share the same drawbacks, except that loadMultipleRevisions on multiple revision ids reduces the amount of queries, which on the other hand has a larger effect the more revisions you reference.

But using loadMultiple is definitely a point, since it will ensure using the entity cache system, which is unfortunately not available for revisions and I think this is out of scope of this issue.

So, why not combining both? Loading default revisions with loadMultiple and everything else with loadMultipleRevisions?

h1nds1ght → created an issue.