Move $user->data into own table

- Move users.data into dedicated user_data table.
- Use columns uid, [module,] property, value.
- Upon user_load(), fetch that into $account.
- Done.

I'll let Morbus bikeshed this issue with his thoughts.

> I'll let Morbus bikeshed this issue with his thoughts.

What the fuck?

@catch: On a second thought, I see where #1 is coming from - seems you thought of users being cached anyway, so hook_user_load() would be sufficient. However, many modules in contrib store data bits per-user, but do not really need a separate database table, and using fields for simple data bits would be a major overhead...

@Morbus: Didn't we talk in IRC about something like enhancing the variables table to store both module and user variables there? IIRC, you argued that content-type variables prefixed/suffixed with the type name are similar (ugly) workarounds already.

Sun: by using the term "bikeshed", you are suggesting that my (as yet written) comments are trivial and stop-motion.

Tagging.

Seems like the main motivation is missing here: Contrib modules currently have no way to easily update or remove their data for users. On sites with 100,000 of users and a few contrib modules installed, you can take a quick peek at users.data to see all kind of garbage in there. All of that gets serialized and unserialized whenever a user is loaded or saved. And that happens often.

I very much agree with the approach of removing the code that reads and writes from this column. I'm fine with either migrating the data to a new table or leaving it where it is. I guess it is clearer to move it.

I do think that contact module uses this column so there is a little more work to do.

Contrib modules can use own table to relate to uid. There is no reason core needs to provide that. It doesn't for nodes, comments, terms, etc. And now we have the fields system which puts the final nail. Custom sites are welcome to add columns to users table using schema alter. And user module will happily load and save for you. So, all in all we don't need this column anymore.

Removing personal tag. Only one person gets to abuse the tagging system like that. If not, we'll have stupid tagging emails going out, and barely meaningful rows in our term_node table.

Some inconvenience with $user->data object on 6.x
#542844: how to update $user->data object

A very careful first stab at this. Not really tested yet. Handle carefully.

wow, simply dropping {user}.data will remove all data stored by contributed modules upon upgrade.

Adding a separate field for every single property will also bloat that table immensely on a site with many contributed modules installed.

What happened to the {user_data} table idea?

Tagging.

Whooo, user cancel uses {user}.data too... awesome. Here is a new patch.

wow, simply dropping {user}.data will remove all data stored by contributed modules upon upgrade.

We are not dropping anything, it's up to the contrib modules to migrate their data.

+++ modules/user/user.install
@@ -198,7 +198,21 @@ function user_schema() {
+      'user_cancel_method' => array(
+        'type' => 'varchar',
+        'length' => 255,
+        'not null' => FALSE,
+        'default' => '',
+        'description' => 'The cancellation method this user have chosen.',
+      ),
+      'user_cancel_notify' => array(
+        'type' => 'varchar',
+        'length' => 255,
+        'not null' => FALSE,
+        'default' => '',
+        'description' => 'The cancellation notification method this user have chosen.',
       ),

Well. That's exactly the reason why I'm arguing for a separate {user_data} table.

These two values will be needed for 0.1% or even less users of a site. They will only be inserted when a user starts the user account cancellation process. Otherwise, they do not exist.

For the other 99.9% of users, we add garbage to every single user load - for the entire lifetime of a site.

Technically, these examples could even work as system variables. But then again, also system variables should be deleted when the user is deleted, but maintaining a relationship between system variables and users sounds totally wonky. Which again brings me back to {user_data}.

This review is powered by Dreditor.

I suggest we stop half-way through: keep the user.data serialized column (and automatically serialize/unserialize it), but stop merging the values from this column in the $user object, and stop automatically storing unknown values from $user into user.data.

Actually, I am quite happy with the latest patches.

@sun - I think you are micro optimizing. If a site installs contact module, it has every reason to think that $user->contact is in the user object. And no matter what table holds that info, it would still be in the user object. Not sure why you think thats garbage. So the only argument then is whether the contact column belongs in users table or not. I think it is fine there, personally. We have a nice schema alter api and we might as well use. No need to add joins if we don't have to. We have supported automatically loading/saving custom columns in the user table for many many years.

well... one of the major problems we also wanted to solve with this issue is that contributed modules have no way to update their data when it is stored in a serialized column. They have to iterate over all existing users, checking every single one, to properly determine whether the user needs to be updated. Not being able to easily maintain the stored data is the flaw of the current system.

There are many contributed modules that just want to add a single property for certain users. We don't want to force them to create an own table just to be able to properly maintain that single property per user.

And there are many other contributed modules that want to add a couple of settings for several users. Some of them might be converted into fields, but some of them not.

Likewise, we have modules that want to temporarily or dynamically attach data to a user. Those do not need an own, permanent storage/table/column, but they still want to carry on certain user data for a limited time-frame. The user account cancellation process in core is a valid example for that. And. The user account cancellation process currently has no way to clean up this data in case the user did NOT confirm the cancellation, since it would need to iterate over all users to find those with that temporary data assigned.

Hence the trivial suggestion in #2. A {user_data} table, keyed by uid, with variable property names and values - similar to the {variable} table, but simply with relation to uid.

So, will each module do its own loading and writing to this new table? I'm don't think core needs to provide this service. If we did do this, we should add a column (or two) to the variable table. it could be module and realm columns. in this case, the realm is uid.

Your suggestion sounds similar to Morbus', who raised the fact that node type specific settings have the same problem (though maybe not as apparent as users.data). However, introducing a realm + realm_id to variable_get/_set/_del and {variable} cannot be considered for D7.

Fresh patch. Including working upgrade path.

$object->data removed from a couple of places, not sure about those removals. However, this also clearly shows how important #585838: Consider a generic $entity->user property really is. I totally hate those half-baked fake "user" objects passed to arbitrary functions and can't count the number of times I needed to hack core to achieve a proper and scalable user loading.

Guys, we quickly need an agreement here. Discussed this issue with moshe in IRC and privately with smk-ka.

We have two approaches here:

1. Removing auto-load and auto-storage of {users}.data. Modules either use their own tables and load their user data when they need it, or they add columns to {users}.data, so it is always loaded.

   Problem: There is the use-case of having to temporarily store data for a user. The only example in core are the two user account cancellation flags - they will only ever be inserted and needed, when a user tries to cancel the own account. For all the other 100% of users on a Drupal site, developers will see cruft in the $user object:

   $user->user_cancel_method = '';
   $user->user_cancel_notify = 0;
   

   We also cannot remove the {users}.data column during upgrade, because modules have to migrate their data.

2. Keeping auto-load and auto-storage of user data, but store it in a separate {users_data} table, basically following the {variable}/{cache} table schema:
   

   uid   name                 value                     serialized
   3     user_cancel_method   delete                    0
   3     user_cancel_notify   1                         0
   3     node_options         a:1:{i:0;s:6:"status";}   1
   

   This nicely leverages the new user_load_multiple() and allows for easy querying and updating of the data. We also neither store nor query nor load any data for users that do not need it.

   Note that also in this approach, we could drop the auto-storage in user_save(), so we just auto-load, but make user account form submit handlers responsible for saving/deleting.

   Also note that the 'serialized' column wasn't yet contained in my last patch.

Thanks for revisiting this.

I'm pretty firmly in #1. We should add new tables for both user cancel and contact. If we don't need those properties frequently, we can just access those tables when we need them, and not on every user load. But really these are indexed queries that are lightening fast - I see no need to avoid them.

And yes, the users.data column can stay where it is for a release. We should not load/save at all.

I object to #2 for the same reasons that everyone objects to profile.module storage.

I'm opposed to #1, because I'm opposed to forcing modules to add a separate table for values like those user_cancel values, because that table will be empty all of the time. The values are only needed for a couple of minutes/hours, days at max, until the user confirmed the account cancellation.

I also do not agree that we're rebuilding Profile module here. That had a totally different purpose, and for that purpose, yes, I completely agree its storage was wrong.

However, if it's really only me who disagrees with #1, then please, let's not hold off this issue. Either way is much better than the {users}.data we have now, because that blocks a proper User API over in #118345: Revamp hook_user_form/_register/_validate/_submit/_insert/_update.

reroll of #20. i'm going to agree with this approach now. in d8, i really do want to offer per user storage of variables so lets not force modules to handle the schema for themselves for d7. this helps with upgrade path to d8.

unfortunately, i am seeing a query to the data column during install but do won't tell me what query. anyway, here is reroll. i probably won't work on this more at this time.

Why not use FieldAPI to store users' variables and {cache_user} (or similar) to old {user}-data

We made a lot of progress in #721436: Remove magical fairy saving of cruft from user_save() but this is still needed.

Are people satisfied with the approach in the last patch? In other words, create a new table which stores name/value per user. I guess I can live it. I'm thinking that contact, user_cancel, and Contrib modules should actively migrate their bits into the new table. That way, we leave behind cruft behind.

I'm thinking that we could add a module column so that uninstall can wipe cruft from the new table. I also think the new table could replace authmap table. Some discussion of the future of authmap at #817118: Remove {authmap} and migrate OpenID entries to their own table

I still think we should use cache_user_data or similar to minimize performance impact of loading many items. On large user base this table (key-value) could grow to tremendous sizes and I think merge queries is overkill

Yeah, I still think that this is needed.

Agreed on the 'module' column. However, I'm opposed to auto-loading/attaching + saving any data. Just provide a per-user storage for simple or temporary variables.

Re-rolled the patch with a couple of adjustments in light of that -- didn't work on any hook_user_load() and hook_user_insert()/_update() implementations.

Looks good to me so far.

I'm not sure, but I think BLOB and has restrictions that we may not like such as not supporting LIKE queries. If so, pick a different one from http://drupal.org/node/159605

+1 subscribe

Now we have key/value storage probably better to store this data there?

Yeah - good call

I see 2 ways of implementation:

1) User module adds {user_data} table and wraps its usage into UserDataStorage as DatabaseStorageExpirable does. It's needed to define a database table
2) Use current {key_value} table but define own $collection user.data

In case of 1) we could take #35 approach and query required values by demand having $collection per user using user.uuid as collection name

I don't think that a key/value store is appropriate for this. This is about persistent site content, and I believe it should and must be queryable in all ways.

I'd recommend to pursue with #35.

FWIW, re-rolled against HEAD.

Suppose at minimum we need a consistent API at this point, I think better to re-use of newly introduced entity-property-iapi and make psedo-property to describe and provide crud for this kind of data

The data column was never queryable. It's a PHP serialized blob. So, moving it to the K-V store (not the expireable one) is something that might make sense. I would use a 'user.data' namespace, the uuid as key and data as value. Should border trivial to implement it.

@chx do you mean to make uuid as $collection name?

Quoting myself from #7 in 2009:

Contrib modules currently have no way to easily update or remove their data for users. On sites with 100,000 of users and a few contrib modules installed, you can take a quick peek at users.data to see all kind of garbage in there. All of that gets serialized and unserialized whenever a user is loaded or saved. And that happens often.

The situation did not change or improve in any way since then. All contributed modules that want to store too simple or temporary data for a particular user account have no way to do so. They could create an own table, each, but enforcing that would lead to a significant increase of tables.

We have a typical example for that in core with User module's account cancellation process, which needs to store two simple values for a limited time-frame for the user that may or may not cancel the account.

The other typical example in core is Contact module. If the module happens to be enabled for some time but gets uninstalled later on, many user accounts will have orphan 'contact' properties for eternity.

I strongly believe this data needs to be queryable, especially to allow the system to clean up obsolete/orphan values.

At the same time, requiring individual tables appears to be a giant overhead for a majority of use-cases throughout core and contrib. I don't like that 1/0 and black/white thinking. Flexinode/Profile module's simple storage design is suitable for certain use-cases.

Sure, stuff that data into K-V. If you need them temporarily then stuff it into the K-V Expirable. users.data can easily become a legacy thing, if we so want.

#47, new DatabaseStorage('user.data')->set($user->uuid, $user->data). I know it's not valid code but it described what I mean.

That doesn't solve the maintenance problem:

How do you delete all stored values of a certain module in all user accounts upon uninstallation of a certain module?

The approach in the patch in #43 solves that in a simple and straightforward way.

Aside from that, I also don't see why a KeyValueStore is considered at all - the stored data is clearly content, there's no need to make the storage backend pluggable, and the data is usually accessed by a specific module for a specific user ID (stopping the anti-pattern of unconditionally loading/saving a serialized dumping ground bin is one major aspect to this after all).

As @Dries tends to say, "the Drupal community is very good in applying patterns", but a KeyValueStore would clearly apply the wrong pattern to something that has different requirements. Let's not make that mistake.

Fixed duplicate user module update function name.
Fixed stale insert query in user_install().

Tests need some love. I'd be fine with proceeding with latest patch.

Let's see how far we get with this.

Added update sandbox to process users.data in chunks of 20 records.
Added user_data_get(), user_data_set(), user_data_del() API functions.
Converted Block, Contact, and Overlay module to User Data API.
Removed $account->data assertions from OpenIDRegistrationTest.
Updated hook_user_presave() phpDoc.
Updated user cancel process for new user data.

Fixed upgrade path failures.
Fixed Contact module's personal contact form user data handling.
Moved handling of user_cancel_* data values in UserStorageController.

Remaining: I wasn't able to figure out yet what's wrong with the user cancel process. Only user.cancel_notify is saved to users_data, but user.cancel_method is not.

1. Merged HEAD + Fixed stale Block/Contact module update dependency numbers.
2. Moved user data saving back into preSave().
3. Improved user_data_get() logic.
4. Use User::id() method instead of $uid in code we are touching.

This patch should pass tests.

This is serious scope creep -- instead of moving to a separate table you are adding a whole new API around it. An API which is not pluggable, not using injection, a totally Drupal 7 API. The API needs to move into a class. Also, I still believe the K-V store is entirely adequate here: the collection could be called user.data.$module , the key could be $uid and then you would get a small blob perhaps an array that you can look at for $name and $value. If you go with this approach then here are useful snippets for you. UserBundle.php:

    $container->register('user.data', 'Drupal\user\DataManager')
      ->addArgument(new Reference('keyvalue'));

In DataManager:

  public function __construct(KeyValueFactory $key_value_factory) {
    $this->keyValueFactory = $key_value_factory;
  }
  function get($module, $uid, $name = NULL) {
    $data = $this->keyValueFactory->get("user.data.$module")->get($uid);
    if (isset($name)) {
      return isset($data[$name]) ? $data[$name] : NULL;
    }
    return $data;
  }

And so on. Deleting a module is simple, deleting a user would require iterating over the modules to delete the value, alas, that's unfortunate, I know. I would be glad to help further.

And $query->addExpression("'block'", 'module'); is invalid SQL. Edit: no, it isn't.

1. Added UserBundle.
2. Replaced procedural functions with user.data service.

#61: user.data_.61.patch queued for re-testing.

Great! After talking to others on IRC (wish you'd be there) "a specialized API does have advantages instead of trying to fit everything into one generic one". One small thing however: could you please add an interface?

Added UserDataInterface.

Fixed lame mistakes.

Hm, that's unfortunate. The API is used in hook_init(), which is called before we initialize the kernel.

Nothing easier to fix than that.

Fixed overlay_init() is invoked before kernel bootstrap.

Fixed TestBase makes too many assumptions on test runner environment.

(i.e., merged in the relevant fixes from #1808220: Remove run-tests.sh dependency on existing/installed parent site)

Patch looks great. My one thought is that the new user_data API has no hooks of its own. I guess changes to user_data are expected to happen within a $user->save()? Works for me.

f
function overlay_user_presave($account) {
   if (isset($account->overlay)) {
-    $account->data['overlay'] = $account->overlay;
+    drupal_container()->get('user.data')->set('overlay', $account->uid, 'enabled', $account->overlay);
   }
 }

I think this belongs in update hook, not presave.

Yeah, I don't see any use-case for invoking hooks there. Modules can integrate with the user entity CRUD operations already.

Final changes:

1. Removed Views data handler for {users}.data.
2. Changed overlay_user_presave() into overlay_user_update().
3. Use $account->id() instead of $uid property where possible. (global $user is not a fully loaded entity.)

This should come back green. The BreadcrumbsTest failures seem to be random test failures in HEAD currently.

#76: user.data_.76.patch queued for re-testing.

#76: user.data_.76.patch queued for re-testing.

Created an issue for that: #1836204: Test failure in Drupal\system\Tests\Menu\BreadcrumbTest

Fixed random test failure in Menu\BreadcrumbTest.

This should come back green and be ready.

#71, #73 and #76 contained that failure and it stayed during retest and I didn't see it occuring anywhere else -- it doesn't seem like a test fluke. If you need, I can give you access to a box which extremely closely mimicks a testbot (Debian squeeze, php 5.3.5 from dotdeb) to countercheck.

Thanks, but I don't think we need that. The randomness is actually reproducible, Simpletest UI vs. run-tests.sh.

The difference is in the {menu_links}.mlid of /node/add — which seems to be caused by the drupal_bootstrap() level change to run-tests.sh.

Quite potentially, something of the test runner leaked into the test environment, which causes hooks to be invoked in a slightly different order, and we never noticed. The only change here is the DRUPAL_BOOTSTRAP_FULL to _CODE in the code path of run-tests.sh that eventually executes TestBase::run(). _FULL only sets up language, path, custom theme, and hook_init().

The bootstrap change is required, since PIFR installs a full-blown parent site with the Standard profile, and the Standard profile contains Overlay module, which in turn implements hook_init(), and within overlay_init(), it checks whether the current user has overlay enabled. That, however, is converted into UserData here, and the UserData service is registered through UserBundle on the kernel/container. DRUPAL_BOOTSTRAP_FULL, however, does not bootstrap a kernel. Consequently, overlay_init() blows up.

Seems like all feedback has been addressed.

@sun - do you think you will implement catch's feedback. would love to review your reroll and push this in. i can't look at users.data for another release!

I'll try. Upgrade path tests are a pain to write, but @catch only seems to ask for some basic/minimal coverage, so hopefully that's not too hard. The part I struggle most with is the "API" part — the current method names and arguments just simply make sense to me from a DX/user perspective. I don't know how they could be separated. The only remotely closest pattern I know of are getBy*, setBy*, deleteBy* methods, but when thinking through that option, 1) there'd be a dozen of methods, 2) each taking different arguments, resulting in 3) an unpredictable/ungrokable API. Compare that to the current patch, which has 3 methods that always take 3 identical arguments in the same order. Hardly beatable?

Sorry for the noise.
Opened a follow up for the removed views integration: #1845526: User data service handler for views
and rerolled against the breadcrumb fix

Another recent reroll.

shame.

Since the data is no longer part of the object, the only way to modify it is to have your own hook_user_update() hook that runs later than the module you want to change, then delete or update the value again. It would've been nice to have kept that access in hook_user_presave() (i.e. updating a userData object that then writes to the database itself later on), but we could discuss that in a follow-up since this stuff is so broken now and is going to be a dump whatever we do with it.

I think I agree that this is a potential issue. IIRC, for some reason I wanted to retain usages of hook_user_presave() in close relation to that, but in hindsight, that wouldn't change a thing.

My own, primary reason for trying to retain _presave() was that it's the only way to support customized Drupal installations, which might opt to expose certain user account settings on the user registration form already (and I personally had many situations in the past in which I was tasked to do exactly that — and, due to our current user account data handling, exposing form controls on the user registration form that normally appear on the edit/profile form only, most often was an easy task.)

That said, I actually think the topic has to be split up some more:

1) We don't want to end up with _presave() tacking arbitrary properties onto an entity, in order to have some fancy collector to collect them in _insert() or _update(). I think we can all agree on that.

2) If 1) is not an option, then we'd be left with a single trashbin à la $entity['data'] in _presave(), which would follow a certain structure and would be consumed by _insert() and/or _update() to perform the necessary writes [potentially even by user.module itself on behalf of all others].

3) The really interesting part about that however is that $entity['data'] technically won't exist unless there is a user account form involved. That is, because we have no reason to load all of those potentially existing per-account settings for every user account on every load. Consequently, they do not exist on $account when it is saved programmatically. Individual user account data values only exist if they have been explicitly loaded and/or assigned.

4) Given that, it almost sounds to me that the only and most natural answer for that problem space is hook_form_alter(). (although I'm not sure how bad or broken that might sound — I'm too involved in this patch to have an outsider perspective)

Feedback welcome.

+ // Process 20 user records at a time. E.g., if there are 10 data keys per user
+ // record, that leads to an insert query with 200 values.

This [module update] could use a $conf variable so that sites can tweak it if it's a problem, I can imagine this taking hours to run on some sites.

To my knowledge, we are not doing that in any other module update. And TBH, I'd leave possible performance optimizations for the upgrade path to later follow-up issues — there's a big risk of premature optimization in that field.

Awww, the warm fuzzy feeling of knowing why you wrote tests :) Found two bugs, the first was that the primary key of the helper table was just on uid and the second was that all queries where missing a name condition. So all these queries failed due to existing primary key exceptions.

Some notes:

- Haven't yet made the limit configurable. There's no variable_get() anymore (will no more be) so I'm not sure what to use there...
- Also added delete statements to remove actually migrated data, so it's easier to see what's left in there.
- Noticed that the upgrade functions are quite fragile. They require the values to be serialized strings and not integers. Not sure how reliable that is.
- Did not add test coverage for block stuff, I think that is being dropped anyway in the block => plugin confirmation so we'd just have to remove it again there.

Just checked the data column manually in 7.x:

data: a:2:{s:7:"overlay";i:0;s:7:"contact";i:1;}

So, integers, not strings. Which means that the upgrade path is wrong. And as I said, switching to integer might be wrong as well. I guess to be save we have to use batch to load all entries and process them in PHP :( Or do some LIKE queries that check on 0/1 only... Or take over the serialized values.

Here's a new patch that keeps those values serialized. That's not as fast, but it's safe. The values will be converted to a scalar once updated by the users.

Thanks for adding upgrade path tests, @Berdir! :)

Can we just use a 'user_update_user_data' variable for now that defaults to 20, 50, or 100?

(20 might have been too defensive on my part... given that these are plain database queries without any other module/PHP code involved, I'd guess that they execute quickly, so 100 might be more appropriate.)

I looked through the other changes, and with that minimal variable adjustment, I think we're back to RTBC here.

Re-roll after the picture and router access issues got it. No interdiff, just fixed the conflicts and moved the update functions to the end.

No variable. Let me explain why:

- update_variable_get() only reads from the database, so you would need to set this with variable_set() in your d7 site before you start the upgrade.
- variable_get() will go a way, so sooner or later, we would need to find a solution and if we don't have one now, then why should we then? The only thing that I can think if is global $conf and that's kinda ugly.
- Not sure why this function needs such a variable and the uuid update right above it (which defaults to just 10 users) doesn't (the same uuid update also runs on nodes and comments with the same limit)
- batch is intelligent enough to automatically call our function again until the time limit of 1s is reached, before it does an actual page refresh. So the difference between doing 20 or 50 for example is really not that big IMHO.
- I don't think it's required. For testing, I created 500k users with db_insert (much faster than entity_save()), added 10 dummy data arrays keys for each one. users table grew to 300+MB. Running the update created the _d7_user_data table with 3 million rows in ~1 minute ( I don't have an exact number but it was fast enough for me to not get bored and watch the progress bar until it was through ;)). This is a fast machine, so it might be slower for most others, but still, that sounds fast enough to me?

That makes total sense to me. I'd even go one step further and say - if you have a gazillion of users in your DB, will you really use the interactive update.php frontend for upgrading that site? Or won't you much rather use non-interactive tools like Drush instead, which are using the non-progressive Batch API to begin with? :)

That said:
Batch API++
yched++
whoever-else-who-originally-designed-it++ ;)

Wow, great job on the upgrade path testing, Berdir!

Looks like catch has sunk his teeth pretty deep into this one, so tossing this on his plate.

I opened #1851902: Review batch size for update functions and make configurable. I made it a normal task with 'revisit before release' tag. I think that adequately describes the current importance. Please up the priority if you think thats not sufficient.

Re-roll, diff on the patch files show that it is identical except of a fieldset/details change that git was able to merge just fine. So back to RTBC. Going to create the follow-up issue now.

Here we go: #1852200: Standardize batch size in update functions and add a global configuration.

Edit: Ups. Missed that there already is an issue for this.

Working on one.

Change notice created: http://drupal.org/node/1852360.

Quick review? :)

In the example code, I would prefer is one checked for the success of db_insert() before doing a db_delete(). Might be that our real upgrade path needs same protection.

Berdir let me know that db_insert() throws an exception on failure, not return FALSE. So we are done here.

Restoring more metadata.

This is the "The total length of a key has to be less than 333 characters" issue, for those following along at home.

+++ b/core/modules/user/user.installundefined
@@ -267,6 +260,54 @@ function user_schema() {
+    'indexes' => array(
+      'module' => array('module'),
+      'name' => array('name'),
+    ),

@@ -788,6 +828,142 @@ function user_update_8013() {
+      'module' => array(
...
+        'length' => 255,
...
+      'name' => array(
...
+        'length' => 128,

Like so.

Note that the update function does not fill in the value for module so you will not break anyone's data this way. We might want a followup to bikeshed how long a modulename we want to allow versus the name field -- say, authmap has a 128 char long module field which I think should be enough but name comes from a PHP array key which had no limits previously...

The previous "official" limit was the 255 characters filename in system table (while we had one. sniff. poor system table.) which would theoretically allow for very long module names. Consider, however sites/somedomain.com/contrib/.module which is already 36 characters allowing only for 219 long character module names. I do not think 204 will hurt anyone.

Re #119 the problem is > PRIMARY KEY (`uid`, `module`, `name`),

Okay apparently I fail at dreditor. :) But yeah, I agree with #120. I think this is the safest way to resolve this by far.

RTBC if it comes back green. We should file a change notice issue for an official module name length cap.

See also: #1186034: Length of configuration object name can be too small.

Committed and pushed to 8.x. Thanks!

See: #1852454: Restrict module and theme name lengths to 50 characters

Coming late to the party :/ -but checking the patch I wonder:

+function contact_user_update($account) {
+ if (isset($account->contact)) {
+ drupal_container()->get('user.data')->set('contact', $account->id(), 'enabled', (int) $account->contact);
+ }

- If this is just storing entity properties/fields, why isn't that issued from the storage controller?
- Why do we expose a separate API for storing user data? When do I use that and when a field?

If that has been clarified already, please provide pointers. I've not been able to find it.

Yeah. I think this should be either moved to be an proper entity field - maybe with lazy-loading, or be removed from $account and various hook_user hooks. Not sure what's the desired direction to take? berdir argued for being able to load it without the entity on IRC.

cross post

Filed #1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL.