Flagging should use some standard entity reference

Would be nice. But, update path. To avoid perhaps create a computed field which provides an entityreference item instead.

+1 to this.

The patch is ready imo. Just a minor point.
+++ b/src/Tests/FlagSimpleTest.php
@@ -117,6 +117,9 @@ class FlagSimpleTest extends FlagTestBase {
+    $this->assertEqual(reset($flaggings)->flagged_entity->target_id, $node_id);

Can we add asserts for target_type and entity properties as well?

Perfect thanks @chx

Nice. Still RTBC.

Thanks for working on this.

dynamic_entity_reference looks like it might be viable at some point in the future. My concern with it previously was the support for Views, but that looks like it might be ok now. Though we probably don't want to be adding a dependency on a module that's not yet stable, so waiting till it lands in core seems best.

1. +++ b/src/Entity/Flagging.php
   @@ -53,7 +63,7 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
   -    return $this->entityManager()->getStorage('flag')->load($this->getFlagId());
   +    return Flag::load($this->getFlagId());
   

   Not sure what this change is for.

2. +++ b/src/Entity/Flagging.php
   @@ -82,7 +92,7 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
   -    return $this->entityManager()->getStorage($flaggable_type)->load($flaggable_id);
   +    return $this->entityTypeManager()->getStorage($flaggable_type)->load($flaggable_id);
   

   Not sure what this change is for.

3. +++ b/src/Entity/Flagging.php
   @@ -116,6 +126,11 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
   +      ->setDescription(t('The entity of which this flag belongs to.'))
   

   Could be worded a bit more clearly -- see other parts of the code such as docblocks for examples.

4. +++ b/src/Entity/Flagging.php
   @@ -136,4 +151,41 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function onChange($name) {
   +    if ($name == 'flagged_entity') {
   +      $this->entity_id->value = $this->flagged_entity->target_id;
   +    }
   +    if ($name == 'entity_id') {
   +      $this->setFlaggedEntity();
   +    }
   +    parent::onChange($name);
   +  }
   

   I'm not sure there's any point in doing this. A flagging applies to one entity only. The only way it can stop applying to that entity is when it is deleted when the entity is unflagged.

5. +++ b/src/Entity/Flagging.php
   @@ -136,4 +151,41 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
   +    $entity_id_item_list = $this->get('entity_id');
   

   The entity_id property will always be a single ID. Choice of variable name seems odd here.

6. +++ b/src/Entity/Flagging.php
   @@ -136,4 +151,41 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
   +    if ($flagged_entity_item_list->isEmpty() != $entity_id_item_list->isEmpty() || $flagged_entity_item_list->target_id != $entity_id_item_list->value) {
   

   The entity_id property can't ever be empty.

7. +++ b/src/Tests/FlagSimpleTest.php
   @@ -117,6 +117,13 @@ class FlagSimpleTest extends FlagTestBase {
   +    $flaggings = \Drupal::entityTypeManager()->getStorage('flagging')->loadMultiple();
   +    $this->assertEqual(count($flaggings), 1);
   +    $flagging = reset($flaggings);
   +    $this->assertEqual($flagging->flagged_entity->getSetting('target_type'), 'node');
   +    $this->assertEqual($flagging->flagged_entity->target_id, $node_id);
   +    $this->assertEqual($flagging->flagged_entity->entity->id(), $node_id);
   +    $this->assertEqual($flagging->flagged_entity->entity->getEntityTypeId(), 'node');
   

   Could we have some comments to explain what's being tested here please?

1. See #2461673: replace FlagService::getFlagById() with Flag::load() where I proposed changing this across the whole module, and #2461661: [meta] refactor and trim down FlagService for reasons I was given not to do that. You're welcome to argue the case for this on #2461661: [meta] refactor and trim down FlagService.

> 2. EntityManager is deprecated

Good point. Can this be fixed in a separate task please? I prefer to keep clean-up commits separate for a history that's easier to understand.

> 4. It's the standard way of doing things. There's nothing in the codebase to move a flagging to a different entity. Yes it makes no sense but it does work code wise.

Moving a flagging to a different entity is a violation of Flag's application logic. We don't provide code for it, and we don't support it. I don't want this code in, because:

a. developers may see it and think that we DO support that, and then write code to do that which breaks
b. developers may see it and think that we DO support that, but haven't completely implemented it, and file feature requests/bug reports requesting that we do.
c. future maintainers of flag will be confused why that code is there, when we don't support that

If you feel it's necessary to prevent future confusion, add a onChange() method which only calls the parent onChange() and has a comment to explain why we don't need to handle a change to a flagging entity's flagged entity.

Code that calls Flagging::create() should be passing in the required values at that point:

    $flagging = $this->entityManager->getStorage('flagging')->create([
      'uid' => $account->id(),
      'flag_id' => $flag->id(),
      'entity_id' => $entity->id(),
      'entity_type' => $entity->getEntityTypeId(),
      'global' => $flag->isGlobal(),
    ]);

We don't yet enforce this but we might -- see #2640944: Move flagging integrity checks from service to Flagging::save() and unflagging checks to a better place. which is related.

By the way, do calculated fields not lazy-load (lazy-calculate) their value, the way they do with Entity API wrappers on d7?

This is ready imo.

+++ b/src/Entity/Flagging.php
@@ -136,4 +154,30 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
+    return parent::bundleFieldDefinitions($entity_type, $bundle, $base_field_definitions); // TODO: Change the autogenerated stub

Comment can be removed.

Looks good.

Just one thing -- I was reading up on setComputed(), and saw this:

 * Bundle fields either have to override an existing base field, or need to
 * provide a field storage definition via hook_entity_field_storage_info()
 * unless they are computed.

Does that mean that we don't need to define this in both baseFieldDefinitions() and bundleFieldDefinitions()?

Fair enough. 'Follow core's pattern' is a principle I frequently follow as well :)

Committed. Thanks!