Update flag counts table schema

> Use the same database constraints as in {flagging}.

That seems like a good idea.

> (e.g. config entities).

We don't currently allow flagging of config entities though. Are there any other kinds of entities with non-numeric IDs? (If not, it's {flagging} that's wrong, not the counts table, maybe?)

Also, we're not currently supporting head-to-head upgrades, so there shouldn't be a hook_update_N().

Hi,

As an additional information and half of what this patch adressed is that the current absence of "not null" => TRUE flag in the members of the primary key (flag_id, entity_type) create an error in the creation definition of table 'flag_counts' using mysql 5.7.9 (mysql Ver 14.14 Distrib 5.7.9, for osx10.11 (x86_64) using EditLine wrapper)

Next exception 'Drupal\Core\Database\DatabaseExceptionWrapper' with message 'SQLSTATE[42000]: Syntax error or access violation: 1171 All parts of a PRIMARY KEY must be NOT NULL; if you need NULL in
a key, use UNIQUE instead: CREATE TABLE {flag_counts} (
`flag_id` VARCHAR(32) DEFAULT NULL,
`entity_type` VARCHAR(128) DEFAULT NULL COMMENT 'The flag type, for example \"node\", \"comment\", or \"user\".',
`entity_id` INT unsigned DEFAULT NULL COMMENT 'The unique ID of the flagged entity, for example the uid, cid, or nid.',
`count` INT unsigned DEFAULT NULL COMMENT 'The number of times this object has been flagged for this flag.',
`last_updated` INT unsigned DEFAULT NULL COMMENT 'The UNIX time stamp representing when the flag was last updated.',
PRIMARY KEY (`flag_id`, `entity_id`),
INDEX `flag_id_entity_type` (`flag_id`, `entity_type`),
INDEX `entity_type_entity_id` (`entity_type`, `entity_id`),
INDEX `flag_id_count` (`flag_id`, `count`),
INDEX `flag_id_last_updated` (`flag_id`, `last_updated`)
) ENGINE = InnoDB DEFAULT CHARACTER SET utf8mb4 COMMENT 'The number of times an item has been flagged.'; Array
(
)

As defined in the example of Schema API it seems we should define the fields part of the primary key as not null although is not fully clear or I didn't find it. This happens on some mysql versions and it's not reproducible in others (probably mysql convert it automatically? Schema API behave in other way?).

Anyhow this patch makes sense to address this bug. I will provide a new patch without the hook_update_n().

Adding new patch setting main fields as not_true and supporting non-numeric IDs.

This patch fixes Flag installation using drush, which is currently not possible.

Changing to RTBC.

entity_type ID's are limited to 32 characters, everything else will throw an exception. If we are changing it, then lets limit to that.

For config entity ID's, the max length is 166 but there is no global limit, so fine with keeping that at 255.

Note that changing this might decrease query/index performance if you do joins against that table.

Not sure if we should split up the not null change into a separate issue, since that is an obvious, simple bugfix, while the other is a task/change that's not quite so simple.

Could be split up, but since the suggested change was so small, here is just an updated patch.

Interdiff is practically changing 255 to 32.

> For config entity ID's, the max length is 166 but there is no global limit, so fine with keeping that at 255.

You can't flag config entities.

I'm no longer clear what this issue is trying to fix.

> Not sure if we should split up the not null change into a separate issue, since that is an obvious, simple bugfix, while the other is a task/change that's not quite so simple.

Someone's done that already: #2657504: Error "All part of primary key must be not null" when installing flag module

> You can't flag config entities.

Content entities can have string ID's too (e.g. UUID). Field API supports that now, entity references do too, comment.module however doesn't as it also has a separate table, similar to flagging.

Reimplemented the patch with the longer entity_id length.

This makes it consistent with Flagging, which I guess fine.

That said, note that the max length for entity_type is 32 (\Drupal\Core\Entity\EntityTypeInterface::ID_MAX_LENGTH) and the max length of a config entity is 166 as mentioned before, although other entity types could possibly have longer ID's, so 255 is probably fine there.

To save storage, you could consider to instead limit entity_type to 32 characters in both places.

To save storage, you could consider to instead limit entity_type to 32 characters in both places.

How would you set the length of the Flagging entity_type field? BaseFieldDefinition::setConstraints()?

string field type has a max_length setting, that automatically adds a validation constraint and uses it for the schema.

What's left to be done here?

+++ b/flag.install
@@ -15,27 +15,26 @@ function flag_schema() {
       'entity_type' => [
         'description' => 'The flag type, for example "node", "comment", or "user".',
-        'type' => 'varchar',
-        'length' => '128',
+        'type' => 'varchar_ascii',
+        'length' => '255',
+        'not null' => TRUE,

Let's set this to the 32 constant as in #15.

+++ b/flag.install
@@ -15,27 +15,26 @@ function flag_schema() {
-        'disp-width' => '10',

What was this for?

I think this is what #15 was suggesting to do. It uses the max length of 166 (config entity ids) for entity_id and the max length of 32 for entity_type.

I've also updated the IS.

I think this is ready to go.