Problem/Motivation

The Entity Usage module is a dependency for the submodule "Paragraphs Library", and there's code that interacts with its API.

Entity Usage is going through a major rewrite and the 2.x branch (which is non BC) will be the only supported version in the future. We should plan to update the code in paragraphs library to support the 2.x branch when it's ready.

Proposed resolution

Update the code to use the 2.x API, and specify the minimum requirement in the info file / composer.json.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcoscano created an issue. See original summary.

miro_dietiker’s picture

Can we somehow depend here on some milestone issue for Entity Usage and postpone on its completion?

marcoscano’s picture

Title: Update paragraphs library to be compatible with Entity Usage 2.x » [PP-1] Update paragraphs library to be compatible with Entity Usage 2.x
Status: Active » Postponed

Sure, we can consider #2956501: Update module documentation for 2.x and release 2.0-alpha1 as the last milestone for a 2.x release in Entity Usage

marcoscano’s picture

This is what the change would look like.

marcoscano’s picture

Title: [PP-1] Update paragraphs library to be compatible with Entity Usage 2.x » Update paragraphs library to be compatible with Entity Usage 2.x
Status: Postponed » Active

Entity Usage 2.0-alpha1 is out :) so this is no longer postponed.

I also created #2957843: Provide a BC layer around 1.x helper methods so things won't break if we directly switch to 2.x. With that, we can even consider if this issue makes sense or not.

In any case, we might update the test because the returned count now is different, so the current tests will fail. Also, the message we show when editing a library item like "Library item used 2 times" will show an accumulated count (from all revisions / translations), which may confuse some users.

miro_dietiker’s picture

So yeah due to the dependency we should decide carefully. However, the Library is pretty new in Paragraphs and Entity Usage has limited adoption, so i guess there is not too much conflict potential if we push the dependency to >=alpha1 like you did.

Now that there is more functionality provided by Entity Usage itself, we might be able to switch to a soft dependency more easily?

IMHO we should focus on making Entity Usage (more) stable instead of trying to be backwards compatible.

Berdir’s picture

+++ b/modules/paragraphs_library/paragraphs_library.module
@@ -178,11 +178,17 @@ function paragraphs_library_form_paragraphs_library_item_edit_form_alter(&$form,
  *
  * @return int
  *   Returns the count of entity usage across all entities.
+ *
+ * @see \Drupal\entity_usage\EntityUsageInterface::listSources()
  */
 function _paragraphs_library_count_usage(array $usage_data) {
   $count = 0;
-  foreach ($usage_data as $entity => $ids) {
-    $count += array_sum($ids);
+  foreach ($usage_data as $entity_type => $entities) {
+    foreach ($entities as $entity_id => $records) {
+      foreach ($records as $record) {
+        $count += $record['count'];
+      }

I'm actually unsure if this should return the sum of usages or the distinct count of actually different sources/entities. So far it didn't matter, but I think it for the message, it makes much more sense to report that 3 actual entities are using it (and clicking on the message will then list throw rows which maybe 10 usages each).

Unsure about the removed test coverage, why is that done?

marcoscano’s picture

Status: Active » Needs review
FileSize
5.96 KB
2.79 KB

I'm actually unsure if this should return the sum of usages or the distinct count of actually different sources/entities.

True, I've adapted the code to account for that, which is certainly better UX.

Unsure about the removed test coverage, why is that done?

In entity usage 2.x we "fixed" the usage controller to skip paragraph items with no parents (in their default revision). The (previous revision) suffix was a side-effect of us not tracking revisions in 1.x, so testing for that doesn't make sense anymore in 2.x. (It may make sense again when #2954039: Fix ::label() logic and add a ParagraphInterface::getOrphanStatus() helper is fixed however).

In any case, I have kept a test that ensures that no "broken" usage is shown (i.e. showing a paragraph row with no parent label).

Status: Needs review » Needs work

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

Berdir’s picture

  1. +++ b/modules/paragraphs_library/tests/src/Functional/ParagraphsLibraryItemTest.php
    @@ -222,9 +223,9 @@ class ParagraphsLibraryItemTest extends BrowserTestBase {
    +    $assert_session->elementContains('css', 'table tbody tr td:nth-child(3)', 'en');
    

    why is this not showing the language label? :)

  2. +++ b/modules/paragraphs_library/tests/src/Functional/ParagraphsLibraryItemTest.php
    @@ -243,10 +244,10 @@ class ParagraphsLibraryItemTest extends BrowserTestBase {
    -    $assert_session->elementContains('css', 'table tbody tr td:nth-child(1)', 'Test content > field_paragraphs (previous revision)');
    -    $assert_session->elementContains('css', 'table tbody tr td:nth-child(2)', 'paragraph');
    -    $assert_session->elementContains('css', 'table tbody tr td:nth-child(3)', 'entity_reference');
    -    $assert_session->elementContains('css', 'table tbody tr td:nth-child(4)', '1');
    +    // No usage is registered here.
    +    // @todo once 2954039 lands, we expect to have a row here indicating that
    +    // the host node references the paragraph in a non-default revision.
    +    $assert_session->elementNotExists('css', 'table tbody tr');
    

    Still don't quite understand this, might be easier to talk this through in chat?

    We are explicitly saving a new revision, so there *should* still be a usage from the previous version? With revisions now explicitly supported, that should work? Isn't #2954039: Fix ::label() logic and add a ParagraphInterface::getOrphanStatus() helper just about what label we are displaying if it doesn't exist in the current revision?

marcoscano’s picture

why is this not showing the language label? :)

Yep, that's another improvement for the list usage page on entity usage :)
(I've added it to https://www.drupal.org/project/entity_usage/issues/2952210#comment-12604196 so this is included as part of the effort to make this page more user-friendly)

About the empty label workaround, as discussed on the chat, the usage is correctly registered, but entity usage just skips the row if the source entity's label is empty. I've opened #2971131: Improve label handling on usage page to fix it more generically in entity usage, in case the label-related part of #2954039: Fix ::label() logic and add a ParagraphInterface::getOrphanStatus() helper is not fixed first.

I've also included here the fix for the test failures due to having our dev dependencies updated.

Status: Needs review » Needs work

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

marcoscano’s picture

Ooh didn't realize we had a WebTest! :) I'll work on updating those next.

marcoscano’s picture

Status: Needs work » Needs review
FileSize
2.06 KB
marcoscano’s picture

Berdir’s picture

Status: Needs review » Needs work
+++ b/modules/paragraphs_library/paragraphs_library.module
@@ -178,11 +178,17 @@ function paragraphs_library_form_paragraphs_library_item_edit_form_alter(&$form,
+  foreach ($usage_data as $entity_type => $entities) {
+    foreach ($entities as $entity_id => $records) {
+      // We only want to register usages by different entities, ignoring usages
+      // accross revisions / translations / fields of the same entity.
+      $count++;
+    }

count($entities) will do the same, without a second loop :)

marcoscano’s picture

Status: Needs work » Needs review
FileSize
8.21 KB
942 bytes

🤦 sorry for missing that.

Thanks!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Tested manually, seems to work fine.

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

miro_dietiker’s picture

Status: Reviewed & tested by the community » Fixed

Very nice then. Time to look forward.

Status: Fixed » Closed (fixed)

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