🇨🇦Canada @dale42

Sorry, did not mean to remove the "Needs tests" tag. Re-adding.

target_revision_id does not appear on every item. I expect this is why the tests are failing.

I have an alternative solution, also attached as a file, that is solving the issue on my system. I don't believe this is the best solution, but I think it will advance discussion on the issue.

$item sometimes has a target_revision_id and sometimes not. This code uses the target_revision_id if available, and falls back to target_id when it isn't. According to the comments "multiple entity load" should be used, so this was used to load the revised version.

Perhaps all items should have a target_revision_id so that all the entities could be loaded by revision id? This would work even if revisions were used, since every entity has a revision id. This would require an upstream change to what ever is producing $item.

There is also EntityReferenceRevisionsFormatterBase. I'm not sure where it fits in the picture.

--- core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php	2024-06-20 09:17:49
+++ core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFormatterBaseFix.php	2024-06-20 09:21:59
@@ -122,6 +122,7 @@
     // "entity reference item lists" being displayed. We thus cannot use
     // \Drupal\Core\Field\EntityReferenceFieldItemList::referencedEntities().
     $ids = [];
+    $vids = [];
     foreach ($entities_items as $items) {
       foreach ($items as $item) {
         // To avoid trying to reload non-existent entities in
@@ -130,13 +131,25 @@
         // at FALSE.
         $item->_loaded = FALSE;
         if ($this->needsEntityLoad($item)) {
-          $ids[] = $item->target_id;
+          if (!is_null($item->target_revision_id)) {
+            $vids[] = $item->target_revision_id;
+          }
+          else {
+            $ids[] = $item->target_id;
+          }
         }
       }
     }
-    if ($ids) {
+    if ($ids || $vids) {
       $target_type = $this->getFieldSetting('target_type');
-      $target_entities = \Drupal::entityTypeManager()->getStorage($target_type)->loadMultiple($ids);
+      $storage = \Drupal::entityTypeManager()->getStorage($target_type);
+      $target_entities = ($ids) ? $storage->loadMultiple($ids) : [];
+      if ($vids) {
+        $target_entities_vids = $storage->loadMultipleRevisions($vids);
+        foreach ($target_entities_vids as $target_entity) {
+          $target_entities[$target_entity->id()] = $target_entity;
+        }
+      }
     }
 
     // For each item, pre-populate the loaded entity in $item->entity, and set

This issue causes the display of invalid information on the Content Moderation "Latest version" tab. Specifically, it does not actually display the data from the latest version, it displays information from the default/published version. Adding the "content moderation" issue tag.

After reading the description of priorities I believe it qualifies a Major, as it is displaying incorrect data.

I'm still on 1.0.0, but encountered the same problem.

My issue was caused by the module not being able to determine what group the content should be created in, so the user never received the appropriate permission via group membership.

My content creation is /node/bundle/add, and the module uses the ->getGroupFromRoute() method from GroupRouteContextTrait, which gets the group id from the path.

Since none of the moderated content entities are used outside of groups, I could add the create draft permission to all users.

I believe Enumeration cases should be Pascal case for the following reasons:

Enumerations are not constants, they are classes

The idea behind making constants upper case, as I understand it, is making them easily distinguishable from variables. A enumeration::case is not a constant, it is it's own unique thing.

For example, a backed enumeration has this feature: Enum::Case->value

Also:

Enumerations can contain constants

Borrowing from the example on the PHP documentation page, https://www.php.net/manual/en/language.enumerations.constants.php, but changing the constant to upper case:

enum Size {
  case Small;
  case Medium;
  case Large;

  public const HUGE = Size::Large;
}

Size::HUGE is easily identified as a constant, vs Size::Large as a case.

If the cases are also upper case, the comparison would be Size::HUGE vs Size::LARGE.

Mixed case is more readable

A reference: https://en.wikipedia.org/wiki/All_caps

These standards are about making the code more readable, Pascal case does that.

dale42 → created an issue.

I did not mean to imply this is cut and dried. The information was provided as another set of data points in support of a change.

And I did not examine the patch. If it is too sweeping then absolutely, more thought should be put into it. As you say, it's easy to mislabel TFA the module and two factor authentication (2fa) the functionality. The more clarity the better for usability.

Do I understand correctly you are open to a patch that changes the abbreviation in a more considered way? Or is this a will not fix?

I would like to add a vote of support for changing the user facing abbreviations to 2FA. And completely agree the module name should not be changed.

The reasons are:

The different between TFA and 2FA is being noticed by module end users. We have a client pushing back on TFA because they were confused and believe their site users will find TFA confusing.

A large, respected standards organizations recognizes 2FA. The glossary of the NIST Computer Security Resource Center has an entry for 2FA: https://csrc.nist.gov/glossary/term/2fa It does not have an entry for TFA: https://csrc.nist.gov/glossary?index=T

Meriam Webster also has an entry for 2FA: https://www.merriam-webster.com/dictionary/2FA

More importantly, comparing search results for "2FA" and "TFA", there are no quality results found for "TFA".

I believe the module would better serve its user base using 2FA in user facing strings. It would be great to decide for maximum user understandability. TFA is a fine abbreviation for the module, just not it's functionality.

We have encountered this issue when quiz takers use the browser back button to go back from the review page to the question page. Feedback from Client: This is allowed behaviour in other quiz systems they use, like H5P, so a natural behaviour for their users.

We were hoping for an easy fix, like using the Javascript beforeunload event to tell the user their quiz results would be lost if they left the page. Unfortunately, a security feature named Sticky Activation means it won't work in this use case. Though the quiz taker has viewed the quiz results page, they typically haven't interacted with it, so the event won't fire.

I think to resolve this, the following would be required:

1. Update QuizQuestionController to allow access on any question in an active quiz
2. Update QuizQuestionAnsweringForm to detect if taker is not on the current question, provide a message, and provide a button to take them to the correct question

Thoughts?

dale42 → created an issue.