FlagCountManager: Call to a member function id() on null

Please review! The changes helped me to successfully migrate my site!

Here is a patch for the time being.

UPDATE: The problem might only appear, when migrating, and should be handled in this issue's related issue. Nonetheless, the empty check might still be a good idea to make sure we do not work with empty variables.

patch-4 only makes the error message not appear during the migration, but the data in the flag_counts table does not update. I've added a new patch which causes behavior when adding a new flag to the database, if there are no related records in the flag_counts table, such a related record will be created.

Here is the diff.

Sorry, the diff seems to be weirdly formatted... @svitlana.miko could you apply your changes to this issue's MR, for better reviewability?

@svitlana.miko could you please incorporate your changes into the MR as @Grevil suggested? So we could finally review this and fix it?

Not doing a migration, but getting this error in the log as well.
#11 failed for me, here is the terminal and .rej info:

sudo -u www-data patch -p1 < 61.patch
patching file src/FlagCountManager.php
Hunk #1 FAILED at 196.
1 out of 1 hunk FAILED -- saving rejects to file src/FlagCountManager.php.rej

+      if ($existing_entity) {
+        $count = $existing_entity->count + 1;
+
+        $this->connection->update('flag_counts')
+          ->fields(['count' => $count])
+          ->condition('entity_type', $entity_type)
+          ->condition('entity_id', $entity_id)
+          ->execute();
+      } else {
+        // Entity does not exist, so insert a new row
+        $this->connection->insert('flag_counts')
+          ->fields([
+            'flag_id' => $flag_id,
+            'entity_type' => $entity_type,
+            'entity_id' => $entity_id,
+            'count' => 1
+          ])
+          ->execute();
+      }
+    }
   }

   /**

Hope this helps.

@alfthecat the patch does not apply to beta 4 but it applies to the current dev branch fine.

Patch file for 8.x-4.0-beta5 from Merge request !61

Tests failed

 [error]  Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'entity_id' cannot be null: INSERT INTO "flag_counts" ("flag_id", "entity_type", "entity_id", "count") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3); Array
(
    [:db_insert_placeholder_0] => favorite
    [:db_insert_placeholder_1] => node
    [:db_insert_placeholder_2] => 
    [:db_insert_placeholder_3] => 1
)

Even after the 61 MR patch I am seeing this error above on Drupal 10.2.5, Flag 4.0.0-beta5. The migration is able to continue but it may not fully solve the problem or creates a new one.

Update when I actually used the MR patch: https://git.drupalcode.org/project/flag/-/merge_requests/61/diffs.patch this works as expected, when I used the patch in #15 that said it was the same as the MR that is when I got the failures. There may be some additional work in the MR that isn't in the patch.

Rerolled the MR to work with 8.x-4.0-beta7

Only change in that last patch is an update of the call to key instead of keys. Key is now deprecated past Drupal 10.2.x per: https://www.drupal.org/node/2205327 →

I believe the base branch already had the update so it stood out when I was reviewing the patch.