Because neither entity names nor bundle machine names are in a database table, there is no official maximum length.

Some entity modules store their bundles in the DB, eg node stores node type with a length of 32, but this is then increased by comment bundles which adds an 11-character prefix.

This leaves contrib modules guessing what to do when they need to store (eg #1709498: lengths of db columns for entity and bundle names are too short).

I suggest that entity API docs should state a maximum length for entity and bundle name length -- say something like 64 -- so anything wishing to store them knows what size field to define.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

willvincent’s picture

I'd suggest 255. That seems reasonable, while still providing adequate protection that bundle names shouldn't run out of available room. I can't imagine a scenario where it would be more than 255, but 64 may well be a bit on the short side.

sun’s picture

Dave Reid’s picture

xjm’s picture

xjm’s picture

Regarding #1, there are very strong reasons to make it as short as possible, either 32 or 64. See the linked issues for more information. Edit: also #347988-117: Move $user->data into own table.

xjm’s picture

Assigned: Unassigned » xjm

Might as well look at this one too.

xjm’s picture

#1871032: Taxonomy module fails to install on MyISAM due to too long schema index caps the index for taxonomy because of #1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL. When we add this, we can probably revert that change.

Let's make the machine name lengths 64 characters maximum. For the upgrade path, let's look at what we defined in D7:

      'type' => array(
        'description' => 'The {node_type}.type of this node.', 
        'type' => 'varchar', 
        'length' => 32, 
        'not null' => TRUE, 
        'default' => '',
      ), 
    'fields' => array(
      'module' => array(
        'type' => 'varchar', 
        'length' => 64, 
        'not null' => TRUE, 
        'description' => "The block's origin module, from {block}.module.",
      ), 
      'delta' => array(
        'type' => 'varchar', 
        'length' => 32, 
        'not null' => TRUE, 
        'description' => "The block's unique delta within module, from {block}.delta.",
      'machine_name' => array(
        'type' => 'varchar', 
        'length' => 255, 
        'not null' => TRUE, 
        'default' => '', 
        'description' => 'The vocabulary machine name.',
      ), 
  $schema['field_config_instance'] = array(
    'fields' => array(
      'id' => array(
        'type' => 'serial', 
        'not null' => TRUE, 
        'description' => 'The primary identifier for a field instance',
      ), 
      'field_id' => array(
        'type' => 'int', 
        'not null' => TRUE, 
        'description' => 'The identifier of the field attached by this instance',
      ), 
      'field_name' => array(
        'type' => 'varchar', 
        'length' => 32, 
        'not null' => TRUE, 
        'default' => '',
      ), 
      'entity_type' => array(
        'type' => 'varchar', 
        'length' => 32, 
        'not null' => TRUE, 
        'default' => '',
      ), 
      'bundle' => array(
        'type' => 'varchar', 
        'length' => 128, 
        'not null' => TRUE, 
        'default' => '',
      ), 

So, it looks like vocabularies are the only core thing that needs an upgrade path for shortening the machine name. We could truncate the machine names over 64 characters and add _0 indices to them if they're non-unique. Since these names can also be used in code, maybe we would also want to provide a BC mapping table that we'd remove in D9.

Edit: I forgot to check other stuff that's been converted to ConfigEntity; we should look at those as well.

xjm’s picture

xjm’s picture

In #1871032-21: Taxonomy module fails to install on MyISAM due to too long schema index, there's a case for hashing machine names that are too long rather than truncating them.

thedavidmeister’s picture

Check comments #19 and #21 of #1871032: Taxonomy module fails to install on MyISAM due to too long schema index for the hashing vs. sequential name discussion.

See comment #10 there for some more discussion about implementing the upgrade path.

Pancho’s picture

Entity / bundle names are often used as part of other compound names, be it hooks or module names or table names or select aliases or paths or what else did I forget?

And as there are many reasons to limit these compound IDs to a reasonable length, the most basic, atomic ID should be as short as possible. The fact that nothing keeps us from having a longer user facing name means there is quite no reason why entities should be longer than absolutely necessary. If we'd limit the machine name of a basic entity to say 14 characters allowing comment or other modules to add their prefix to some bundles, then we're easily at 32 characters. It shouldn't be much longer IMHO.

My proposal: 14 characters soft maximum in the UI, 32 hard maximum for compounds.
Short is beautiful.

joachim’s picture

14 is VERY short when you want a module prefix. I am sure Commerce will go over that with things like commerce_order_line_item.

Pancho’s picture

Hmm, that's chasing its own tail, but yeah, you're obviously right. My proposal would have been an elegant convention, but I agree it went too far.
We should still really try hard for very short entity machine names. This all tends to pile up so much in compounds.

But where to start?
If it should be possible to have the module name prepend the name of entities it provides, and at the same time it should be possible for a module name to match the entity name it provides, and then there would be modules that add a pre- or postfix to the entities they work with, then we are in a vicious circle.

How do we break out of this?
By a carefully thought-out set of conventions? Or by some arbitrary hard limit that everybody somehow has to cope with? Probably a mix of the two would be best, so conventions would support developers in making the right choice so they can keep all namings consistent without hitting the hard limits at a later point.

How could that look like?
We could say that the names of modules providing (multiple) entities with the module's name as a prefix, have to be short (say max. 12 characters for the module). The resulting entity name shouldn't be all to long either (say max. 24), to allow third-party modules to pre- or postpend their name as well. We'd end up with a maximum of say: 36 or 40, and that would be a hard limit.
Modules that only provide a single entity they share their name with, might go up to 24 characters for both module and entity name, but no more.
Developers that go beyond these conventions risk conflicts with third party modules because these couldn't enforce their own naming standards without hitting the hard limit.

We'd need to extend these conventions to field names etc. to come up with a set of well-coordinated conventions not patchwork where the one identifier is too long for the other identifier or the other way around. And therefore we'd need to extensively collect use cases and de facto conventions.

This is unpleasant business, that this would only be the start of, but I think we can't run away from this without going insane at other points. Just take a look at this line:
SELECT node_node_data_field_parent_node_node_data_field_pbcontent_year.field_pbcontent_year_value AS node_node_data_field_parent_node_node_data_field_pbcontent_year_field_pbcontent_year_value,
This is taken from the OP in #571548: Identifiers longer than 63 characters are truncated, causing Views to break on Postgres, and while this example is about fields and doesn't even contain an overly long identifier, the first sight should make clear how large our problem gets when dealing with really long identifiers.

Xano’s picture

High and/or undefined maximum lengths can cause problems with primary keys, for instance (See #1924796: {paymentreference} index is too long).

A maximum of 64 characters for entity types and bundles makes sense, because in Drupal 8 this allows for 14 more characters on top of the 50-character module name limit, so prefixing is still possible.
As Drupal databases are UTF-8, I was told that the lengths of columns in an index will have to be multiplied by 3 to get the actual byte count. This means that a VARCHAR(64) column takes up 64*4=192 bytes in the index.

In my particular use case, the index consists of the entity type, the bundle and the field name. The entity type and bundle will then only take up 384 bytes in the key, with the remainder left for the field name.

Xano’s picture

Bump.

Xano’s picture

Shy bump?

thedavidmeister’s picture

@Xano, can you roll a patch others can review?

Xano’s picture

Do we have consensus yet?

andypost’s picture

chx’s picture

I would say that due to upgrade path we do not add new limits.

We didn't add one in the entity storage patch which puts the entity type into the table name and if it's too long, cuts it off at -10 and uses the last 10 for a hash to make the table name unique.

So, this issue is a won't fix IMO.

chx’s picture

Status: Active » Closed (won't fix)
xjm’s picture

Status: Closed (won't fix) » Active

1. We need an issue to add that index; why was this closed wontfix? 2. This impacts more than database storage; it also affects configuration object names.

andypost’s picture

xjm’s picture

Priority: Normal » Critical
Issue tags: +beta blocker
xjm’s picture

Issue tags: +Naming things is hard
xjm’s picture

Issue tags: +Configuration system
Berdir’s picture

Status: Active » Needs review
FileSize
6.97 KB

It's not clear to me what exactly we need to do here?

Started by updating a few schema definitions for entity_type and bundles.

Not sure what to do about comment bundle, though, it's a compound ID of the commented entity_type and the field name.

alexpott’s picture

The problem with 64 is that this could possibly take us over the current 250 config object name length (this length is based both on cache key and filesystem restrictions). The field instance config object name is the provider, entity type id, target_entity_type_id, target_entity_type_bundle, and field name. If another module with a name that is the max length for an extension (50) then the longest possible name would by 50 + 64 + 64 + 64 + 32 (field name max) which is 274.

The comment prefix is a bad idea it makes max length enforcement impossible - this is the same for way the language overrides currently work :(

yched’s picture

Dunno if its applicable here, but field db storages solves a similar problem of max string length for table names, using hashes when exceeding a given length. There also is a "multiple parts each with variable lengths" aspect over there, but at least, as opposed to here, the number of parts is fixed, which probably makes it a tad simpler :-(

xjm’s picture

I think we should be declaring a constant on Entity itself for the maximum length, and throwing an exception on save if it's longer, similar to #1852454: Restrict module and theme name lengths to 50 characters or fields. And I think it needs to be 32 characters, for all the reasons outlined in #1191434: Standardize entity type is a maximum of 32 characters (not 64 or 128) and #28.

Berdir’s picture

#32 is fine with me, will update the patch.

Doesn't solve the problem for comments though, unless we're ok with making it impossible add comment fields to entity type whose ID is longer or equal than 32 - length(__field_a), as that would be the shortest comment field name you could come up without changing the hidden field setting to make the field_ prefix optional If you do, then it's 6 characters more that's possible, assuming a 1-character comment field name ;)

Not sure I follow the "Constant on Entity and verify it on save". We could only do that for config entities that serve as bundles (we could have a BundleConfigEntityBase for vocab, node type, custom block type, and category with a lot of what they do unified). Comments have no explicit bundle entity and other entities can be longer, (that's then what #2220757: Limit length of Config::$id to something <= 166 characters would be a about)

andypost’s picture

Another way to solve comment "field_id" issue is to implement some kind of config to hold mapping from "{entitu_type_id}__{field_name}" to some hash.
Current implementation is fragile because supposes that {entity_type_id} does not have 2 underscores.

jessebeach’s picture

+++ b/core/modules/block/custom_block/custom_block.install
@@ -44,7 +44,7 @@ function custom_block_schema() {
-        'length' => 32,
+        'length' => 64,

This should be 50 chars instead of 64.

xjm’s picture

I discussed #31 with @berdir -- we agreed it might make the most sense to de-scope bundle names from this issue, and/or file a followup issue to fix comment bundles.

The goal of #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term) is to make the entity type ID always composed of the module name, a dot, and then a unique identifier for the type (e.g. taxonomy.term or field.instance). Since the module name is already capped at 50, I think it's safe to make the second part (what's currently the config_prefix for config entities) capped at 32, as discussed in #2120003: [META] Create sensible limits for the maximum length of configuration object filename components.

Given that, it might also make sense to postpone this on #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term). What do you think?

Berdir’s picture

Status: Needs review » Needs work

We decided over there, so we will simply go with 32/332 here, this should be a fairly simple patch to update.

We also discussed comment a bit and sait that a field settings would be nice as it would also allow to use the same comment bundle for different bundles, something that is right now not possible. I guess that setting would need to be read-only then. We will do that in a separate issue.

zserno’s picture

Status: Needs work » Needs review
FileSize
7.45 KB

Rerolled patch from #27 in respect of comment #33 and using the 32 character limit. Might be missing some more core entity types though.

yched’s picture

While working with @chx on #1497374: Switch from Field-based storage to Entity-based storage last summer, restricting the lenghts of entity types and bundles was deemed impossible because of D7 upgrade path and existing, unconstrained strings in D7 sites. Are things different now ?

Status: Needs review » Needs work

The last submitted patch, 36: standardize-bundle-length-1709960-36.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: standardize-bundle-length-1709960-36.patch, failed testing.

dasrecht’s picture

Status: Needs work » Needs review
dasrecht’s picture

I ran the tests in question locally and they worked well. Queued the patch for re-testing.

Berdir’s picture

+++ b/core/modules/block/custom_block/custom_block.install
@@ -44,7 +44,7 @@ function custom_block_schema() {
         'description' => 'The type of this custom block.',
         'type' => 'varchar',
-        'length' => 32,
+        'length' => 50,
         'not null' => TRUE,
         'default' => '',

Why 50?

zserno’s picture

@berdir As per #33. I think it's because module names can be 50 characters long.

Status: Needs review » Needs work

The last submitted patch, 36: standardize-bundle-length-1709960-36.patch, failed testing.

zserno’s picture

@dasrecht That's an interesting one. I spent some time on testing it locally as well (on php 5.4). I could reproduce the issue but not on every test run, seems to happen randomly.
So testTermAutocompletion() in core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTest.php is trying to check if two predefined terms are showing up in autocomplete. These terms are the following:

  1. 1st term: '10/16/2011'
  2. 2st term: '10/17/2011'

Test sends a GET request to the autocomplete callback with query string '10/' and then expects the result to contain these 2 terms in this very order. This seems to not always happen. Sometimes the result is the following:
{"value":"10\/17\/2011","label":"10\/17\/2011"},{"value":"10\/16\/2011","label":"10\/16\/2011"}]
So that's why this test fails. I could trace these lines back in git to #675446: Use jQuery UI Autocomplete, comment #236.

Is this a known issue or we're the first who got bit by this? :)

xjm’s picture

#33 was incorrect; it should be 32 characters.

zserno’s picture

Discussed it with xjm on IRC:

I'd like to add a class constant to EntityTypeInterface that defines that max length, and then use that in the hook_schema(), and throw exceptions on installations when it is exceeded, similar to what we did in #1852454: Restrict module and theme name lengths to 50 characters. Also, I think we want to switch bundles off into a separate issue, right Berdir?

So this issue is now focusing on declaring a maximum length for entity type names (bundle names will come later in a separate issue). Unlike #1852454: Restrict module and theme name lengths to 50 characters, this is a somewhat more complex task:

  1. Where could I check if an entity type's name is shorter than 32 before this entity type is registered? During module install? E.g. taxonomy module defines the 'taxonomy_term' entity type which is 13 chars long. Is there a central place where I could check such thing and throw an exception if needed?
  2. Are we talking about content entities only?
Berdir’s picture

Let's discuss this in person, visit me in the Margrathea room.

zserno’s picture

Status: Needs work » Needs review
FileSize
10.87 KB

We agreed with Berdir to include bundle names in this issue, but ignore comment for now as that's more tricky.
Here's a patch that throws an EntityMalformedException (maybe not the best choice?) if entity type ID or bundle name are longer than 32 characters.

Status: Needs review » Needs work

The last submitted patch, 50: standardize-bundle-length-1709960-50.patch, failed testing.

Berdir’s picture

Looks good already, some feedback below, mostly minor things.

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -333,6 +336,18 @@ public function getEntityType() {
    +        throw new EntityMalformedException(String::format(
    +          'Attempt to create a bundle with an ID longer than @max characters: %ID.', array(
    +            '@max' => EntityTypeInterface::BUNDLE_MAX_LENGTH,
    +            '%ID' => $this->id(),
    

    Don't use % placeholders in exception messages, they are sometimes incorrectly encoded. Also should be lowercase, I'd suggest @max_length and @id.

    EntityMalformedException is a pretty good match here I think, because this is a an entity that is malformed :)

  2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -182,8 +183,20 @@ class EntityType implements EntityTypeInterface {
        * @param array $definition
        *   An array of values from the annotation.
    +   * @throws EntityMalformedException
    +   *   When attempting to instantiate an entity type with too long ID.
    

    the @throws should use the full namespace, should have an empty empty between it and the @param. Also, the description needs to be a full sentence, something like "Thrown when..."

  3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -182,8 +183,20 @@ class EntityType implements EntityTypeInterface {
    +    if (Unicode::strlen($definition['id'] > static::ID_MAX_LENGTH)) {
    

    Looks like the closing ) is at the wrong place here, you're doing a strlen() on the comparison.

    Wondering if we need to use the Unicode version but it probably doesn't hurt.

  4. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -182,8 +183,20 @@ class EntityType implements EntityTypeInterface {
    +      throw new EntityMalformedException(String::format(
    +        'Attempt to create an entity type with an ID longer than @max characters: %ID.', array(
    +          '@max' => static::ID_MAX_LENGTH,
    +          '%ID' => $definition['id'],
    +        )
    +      ));
    

    Same as above about the exception message.

    Here I think the exception class doesn't match that well. Let's create a new one, EntityTypeIdLengthException.

  5. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -18,6 +18,12 @@
       /**
    +   * The maximum length of ID and bundle name, in characters.
    +   */
    +  const ID_MAX_LENGTH = 32;
    +  const BUNDLE_MAX_LENGTH = 32;
    

    The bundle needs a separate documentation block.

Berdir’s picture

Status: Needs work » Needs review
FileSize
13.48 KB
2.67 KB

Here's a patch with the unit test fix for CommentLockTest, I also added test coverage for the id check which is currently failing due to the wrong check. Should pass when you fix it but will need to be updated for the correct exception message.

jessebeach’s picture

Assigned: xjm » jessebeach
zserno’s picture

Assigned: jessebeach » zserno
jessebeach’s picture

After discussion with zserno and Berdir, we're making the following changes.

  • Introducing \Drupal\Core\Entity\EntityTypeIdLengthException and \Drupal\Core\Entity\ConfigEntityIdLengthException.
  • Replacing \Drupal\Core\Entity\EntityMalformedException in Drupal\Core\Entity::presave with \Drupal\Core\Entity\ConfigEntityIdLengthException
  • Replacing \Drupal\Core\Entity\EntityMalformedException in Drupal\Core\Entity::__construct() with EntityTypeIdLengthException.
  • Removing \Drupal\Core\Entity\EntityMalformedException altogether if possible.

Status: Needs review » Needs work

The last submitted patch, 53: standardize-bundle-length-1709960-53.patch, failed testing.

zserno’s picture

tim.plunkett’s picture

Please put EntityTypeIdLengthException in Drupal\Core\Entity\Exception, and consider putting ConfigEntityIdLengthException in Drupal\Core\Config\Entity\Exception.

See #2228505: Clean up Entity exception classes

zserno’s picture

Status: Needs work » Needs review
Related issues:
FileSize
6.59 KB
16.87 KB

Reroll according to #53, #56 and #59.

I am aware of 2 phpunit errors. Both occur in ConfigEntityTypeTest where they trigger our new EntityTypeIdLengthException because of this line:
'id' => $this->randomName(59),
so our exception works at least. :) Not sure how to fix it though...

Status: Needs review » Needs work

The last submitted patch, 60: standardize-bundle-length-1709960-60.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
20.29 KB
4.77 KB

Oh lol, I'm so stupid :)

I made us add test coverage for that in the other issue, adding at least 3 unnecessary loops but completely forgot that we want to limit the ID to 32 characters o.0. Well, nobody else thought it through either. Don't believe everything I say :p

Removed all the id stuff from that test and simplifed the test to no longer use providers.

Changes in the interdiff look great, this might be ready?

Status: Needs review » Needs work

The last submitted patch, 62: standardize-bundle-length-1709960-62.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
22.11 KB
1.65 KB
+++ b/core/modules/taxonomy/taxonomy.install
@@ -75,8 +77,8 @@ function taxonomy_schema() {
     ),
     'indexes' => array(
-      'taxonomy_tree' => array(array('vid', 64), 'weight', 'name'),
-      'vid_name' => array(array('vid', 64), 'name'),
+      'taxonomy_tree' => array('vid', 'weight', 'name'),
+      'vid_name' => array('vid', 'name'),
       'name' => array('name'),

It seems that removing the 64 character string length limit from the taxonomy_tree index value caused the order of the taxonomy terms returned from an autocomplete query to change.

I've made the tests result-order independent.

yched’s picture

No-one answered #37 ? How do we deal with length restrictions on data migrated from D7 where those restrictions didn't exist ?

Berdir’s picture

@yched: Can we open a non-beta blocking issue for solving that for migrate? I think doing it isn't the question, so we need to solve it anyway, but that part is not as urgent.

This is also tagged as backport, but I don't think that is possible.

jessebeach’s picture

This is also tagged as backport, but I don't think that is possible.

Right, we can't retroactively limit the length of bundle and entity machine names in D7 now.

Removed the tag.

I added an issue for the migrate problem: #2229209: Restrict entity ID and bundle machine name string length to 32 characters.

moshe already commented:

The migration team will sort this. I vote for automatic truncate, with uniqueness preserved. So if we run into articles and articles_red and we have to truncate we do articl and articl1. We run into this in Migrate a lot.

Code review

Here is the migration followup #2229209: Restrict entity ID and bundle machine name string length to 32 characters, mentioned in #65/#66.

+++ b/core/modules/block/custom_block/custom_block.install
@@ -161,7 +163,7 @@ function custom_block_schema_0() {
         'not null' => TRUE,

+++ b/core/modules/comment/tests/Drupal/comment/Tests/Entity/CommentLockTest.php
@@ -7,6 +7,7 @@
 /**
@@ -74,10 +75,16 @@ public function testLocks() {

@@ -74,10 +75,16 @@ public function testLocks() {
     $comment->expects($this->any())
       ->method('getThread')
       ->will($this->returnValue(''));
-    $comment->expects($this->at(0))
+
+    $entity_type = $this->getMock('\Drupal\Core\Entity\EntityTypeInterface');
+    $comment->expects($this->any())
+      ->method('getEntityType')
+      ->will($this->returnValue($entity_type));
+    $comment->expects($this->at(1))
       ->method('get')
       ->with('status')
       ->will($this->returnValue((object) array('value' => NULL)));

Ok, so, I grok the introduction of the constants and the config prefix test changes. It all looks good.

The only part of the patch I'm not understanding is the code above. What are we testing here? Maybe a couple comments would be helpful :) Once we get those, we're ready to go with this patch.

Berdir’s picture

That snippet there doesn't test anything, we're just fixing an existing test.

The problem is that ConfigEntityBase::preSave() now expects that it can call $this->getEntityType()->getBundleOf(), which is a perfectly valid assumption when running for real, but in a unit test, we need to mock that call now. This (changes breaking unit tests) unfortunately happens *very* frequently, which can partially be attributed to our very coupled code base but not in this case :)

andypost’s picture

for comment module I'm going to make "field_id" mix to a entity reference field pointing to field instance entity, so this not a case soon

Berdir’s picture

zserno’s picture

#2220757: Limit length of Config::$id to something <= 166 characters is just in, together with \Drupal\Core\Entity\ConfigEntityIdLengthException. Rerolling...

Status: Needs review » Needs work

The last submitted patch, 64: standardize-bundle-length-1709960-64.patch, failed testing.

zserno’s picture

Status: Needs work » Needs review
FileSize
21.45 KB
1.69 KB

Removed \Drupal\Core\Entity\ConfigEntityIdLengthException from patch.

Status: Needs review » Needs work

The last submitted patch, 73: standardize-bundle-length-1709960-73.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
22.84 KB
1.56 KB

*grml* Unit tests *grml*

zserno’s picture

Assigned: zserno » Unassigned

Returned green, looking good to me, unassigning.

jhodgdon’s picture

Ummmm... The max length here -- doesn't it actually need to be in bytes, not characters? If it's stored in a DB field of a particular length, wouldn't bytes be the requirement?

Berdir’s picture

We always specify characters, bytes is based on the actual database/charset, we can not control that, that's why the index length limit validation is almost impossible to do "correctly".

This is at least as much about configuration files as it is about database, but it doesn't affect them directly, so we mostly only touch schema/field definitions. By going with 32, we ensure that all configuration entities that are attached to a entity_type/bundle combination only need to reserve "32 + . + 32" characters for that and can use whatever ls left for their own identifiers.

After all, this issue is part of the configuration file name length issue now.

The clean-up on the indexes is part "no longer needed because it's now shorter than the index length" and part "4 character length cleanup" which is ... weird :)

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -336,6 +338,18 @@ public function getEntityType() {
+        throw new ConfigEntityIdLengthException(String::format(

Why are we throwing a ConfigEntity exception from Entity? Should this move to ConfigEntityBase?

Berdir’s picture

Good question, I think I said that to @zserno when we discussed this as well, but don't remember exactly. Is it a strict requirement that bundle entities must be config entities? I guess yes as fields and so on need to be able to depend on them and being deployed together?

effulgentsia’s picture

Is it a strict requirement that bundle entities must be config entities?

I think it should be, but I don't know if we currently enforce that anywhere. We now have ConfigEntityType and ContentEntityType subclasses, so possibly we should move EntityType::getBundleOf() to ConfigEntityType? That would also require creating ConfigEntityTypeInterface.

tim.plunkett’s picture

Note that #2204697: Move getConfigPrefix() to ConfigEntityTypeInterface adds ConfigEntityTypeInterface

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

In that case, opened #2231017: Move EntityType::getBundleOf() to ConfigEntityType as a non-beta-blocking follow up. No sense in holding up a beta blocker on something relatively minor like that. RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

I think this needs a change record since vid's in Drupal 7 could be longer than 32.

jessebeach’s picture

Assigned: Unassigned » jessebeach

I'm working on the change notice.

jessebeach’s picture

Status: Needs work » Reviewed & tested by the community

Change notice is ready. https://drupal.org/node/2232665

jessebeach’s picture

  • Commit 6edcdee on 8.x by webchick:
    Issue #1709960 by Berdir, zserno, jessebeach | joachim: Declare a...
webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed to 8.x. Thanks!

I'm not sure if this actually can be backported, but moving it to 7.x nonetheless.

Berdir’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Fixed
Issue tags: -Needs backport to D7, -Needs change record

We apparently failed to actually remove the backports tag in #66/#67 but definitely wanted to, I don't see how this can be backported. So moving to fixed.

Yay!

xjm’s picture

I don't think the change record needs to go into quite so much detail about config object names. We're also doing this for other reasons, like to stop breaking database indexes. Other than that, looks great!

xjm’s picture

Maybe a format like https://drupal.org/node/2014073 would be useful. Brief descriptions, rather than detailed ones, of the reasons we're limiting these values and why we chose the limit we did (they are used to compose longer values for config object names, cache keys, database indexes, etc. that are limited to 255 or 333 characters).

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

bdone’s picture

added #2244775: Declare a maximum length for contact category machine name as a follow up that might have been missed here.

dqd’s picture

Version: 8.0.x-dev » 8.5.x-dev

For some strange reason it is still possible to circumvent the limitation. WSOD: Drupal\Core\Entity\Exception\EntityTypeIdLengthException: Attempt to create an entity type with an ID longer than 32 characters: reusable_entityreference_bundle_type. in Drupal\Core\Entity\EntityType->__construct() (line 299 of core/lib/Drupal/Core/Entity/EntityType.php).

While this is an error caused by using ECK, in summary it belongs to the fact that it is still "possible" to try creating entity types with machine names longer than 32 characters leading to a WSOD and broken project database (unless you know how to fix this manually). Maybe I am wrong, but Google told me that this is the only issue regarding this topic and from my understanding the underlying problem needs to be solved in core. Not sure but I think we better should reopen this here before creating another one related to this. Until somebody lauters me, that I am wrong with my assumption that this is related.

Berdir’s picture

No, this is working exactly as it should, it prevents you from going further. If ECK allows you to use such an ID then that's the fault of that module not of core.

dqd’s picture

this is working exactly as it should, it prevents you from going further.

That's right, agreed.

But the modules requests obviously did "something" BEFORE this prevention comes into account, otherwise you wouldn't have the WSOD afterwards. Shouldn't we force any added function which hooks into any core API then to change the order of its group of calls/requests caused/made by a user input the way that THIS CHECK has to be the first to prevent that something is half the way when this mechanism prevents the group of calls or requests going further?

What I am after is the question, how much can we do on core level to prevent modules or any other function doing it "wrong". Something like: "The ID (machine name) has to be checked against requirements before other parts of this functions in the request are allowed." ... This came in mind by kind of "API first" rock solid thinking.

But maybe this is impossible to do on this level since we maybe do not know what this call is part of early enough. This is not an easy task and possibly an impossible one, since we can not know what the ID creation is part of on this level, until we invent a certain "group of events map" logic which will be checked against all parts of it first before it gets acceptance to run (door keeper).

Berdir’s picture

No, we can not. We don't know anything about ECK or dynamically defined entity types. As far as we're concerned, entity types are classes with annotations. We can't detect that in advance and prevent it, all we can do is fail hard with a clear error when it exists.

Preventing this situation is the job of ECK.

dqd’s picture

Ha. My EDIT was half the way down but came too late for your answer. :) Yes, and yes, as you sad. That's also my conclusion so far. Thanks Berdir for taking the time to share some thoughts on this. Issue created for ECK here.

dqd’s picture

all we can do is fail hard with a clear error when it exists

Well, in this case it breaks the Drupal installation completely with no way out. This is indeed a hard fail...