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.

Files: 
CommentFileSizeAuthor
#75 standardize-bundle-length-1709960-75-interdiff.txt1.56 KBBerdir
#75 standardize-bundle-length-1709960-75.patch22.84 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,042 pass(es).
[ View ]
#73 standardize-bundle-length-1709960-73-interdiff.txt1.69 KBzserno
#73 standardize-bundle-length-1709960-73.patch21.45 KBzserno
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#64 interdiff.txt1.65 KBjessebeach
#64 standardize-bundle-length-1709960-64.patch22.11 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch standardize-bundle-length-1709960-64.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#62 standardize-bundle-length-1709960-62-interdiff.txt4.77 KBBerdir
#62 standardize-bundle-length-1709960-62.patch20.29 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,941 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#60 standardize-bundle-length-1709960-60.patch16.87 KBzserno
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,874 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#60 standardize-bundle-length-1709960-60-interdiff.txt6.59 KBzserno
#53 standardize-bundle-length-1709960-53-interdiff.txt2.67 KBBerdir
#53 standardize-bundle-length-1709960-53.patch13.48 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,820 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#50 standardize-bundle-length-1709960-50.patch10.87 KBzserno
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#36 standardize-bundle-length-1709960-36.patch7.45 KBzserno
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,736 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#27 standardize-bundle-length-1709960-1.patch6.97 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,455 pass(es).
[ View ]

Comments

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.

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.

Assigned:Unassigned» xjm

Might as well look at this one too.

#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.

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.

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.

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.

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.

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: truncated identifiers with more than 63 chars, causing Views to break, 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.

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.

Bump.

Shy bump?

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

Do we have consensus yet?

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.

Status:Active» Closed (won't fix)

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.

Priority:Normal» Critical
Issue tags:+beta blocker

Issue tags:+Naming things is hard

Issue tags:+Configuration system

Status:Active» Needs review
StatusFileSize
new6.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,455 pass(es).
[ View ]

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.

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 :(

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 :-(

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.

#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)

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.

+++ 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.

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?

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.

Status:Needs work» Needs review
StatusFileSize
new7.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,736 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

+++ 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?

@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.

@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? :)

#33 was incorrect; it should be 32 characters.

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

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

Status:Needs work» Needs review
StatusFileSize
new10.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new13.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,820 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.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.

Assigned:xjm» jessebeach

Assigned:jessebeach» zserno

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.

Posted a followup for comment bundle machine names: #2228763: Declare a maximum length for comment bundle machine names.

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

Status:Needs work» Needs review
Related issues:
StatusFileSize
new6.59 KB
new16.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,874 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new20.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,941 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new4.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.

Status:Needs work» Needs review
StatusFileSize
new22.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch standardize-bundle-length-1709960-64.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.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.

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

@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.

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.

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 :)

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

#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.

Status:Needs work» Needs review
StatusFileSize
new21.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new1.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.

Status:Needs work» Needs review
StatusFileSize
new22.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,042 pass(es).
[ View ]
new1.56 KB

*grml* Unit tests *grml*

Assigned:zserno» Unassigned

Returned green, looking good to me, unassigning.

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?

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 :)

+++ 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?

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?

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.

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

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.

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.

Assigned:Unassigned» jessebeach

I'm working on the change notice.

Status:Needs work» Reviewed & tested by the community

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

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

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.

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!

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!

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.

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