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.
Comment | File | Size | Author |
---|---|---|---|
#75 | standardize-bundle-length-1709960-75-interdiff.txt | 1.56 KB | Berdir |
#75 | standardize-bundle-length-1709960-75.patch | 22.84 KB | Berdir |
#73 | standardize-bundle-length-1709960-73-interdiff.txt | 1.69 KB | zserno |
#73 | standardize-bundle-length-1709960-73.patch | 21.45 KB | zserno |
#64 | interdiff.txt | 1.65 KB | jessebeach |
Comments
Comment #1
willvincent CreditAttribution: willvincent commentedI'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.
Comment #2
sunSuper closely related: #1186034: Length of configuration object name can be too small
Comment #3
Dave ReidAlso see #1191434: Standardize entity type is a maximum of 32 characters (not 64 or 128).
Comment #4
xjmAlso: #1852454: Restrict module and theme name lengths to 50 characters
Comment #5
xjmRegarding #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.
Comment #6
xjmMight as well look at this one too.
Comment #7
xjm#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:
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.
Comment #8
xjmSee: #1871032-10: Taxonomy module fails to install on MyISAM due to too long schema index
Comment #9
xjmIn #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.
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commentedCheck 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.
Comment #11
PanchoEntity / 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.
Comment #12
joachim CreditAttribution: joachim commented14 is VERY short when you want a module prefix. I am sure Commerce will go over that with things like commerce_order_line_item.
Comment #13
PanchoHmm, 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.
Comment #14
XanoHigh 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.
Comment #15
XanoBump.
Comment #16
XanoShy bump?
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commented@Xano, can you roll a patch others can review?
Comment #18
XanoDo we have consensus yet?
Comment #19
andypostSeems duplicates #1191434: Standardize entity type is a maximum of 32 characters (not 64 or 128)
Comment #20
chx CreditAttribution: chx commentedI 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.
Comment #21
chx CreditAttribution: chx commentedSame as in #1191434-7: Standardize entity type is a maximum of 32 characters (not 64 or 128) : add an index length and carry on.
Comment #22
xjm1. 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.
Comment #23
andypostComment #24
xjmBeta-blocking as a required part of #2120003: [META] Create sensible limits for the maximum length of configuration object filename components.
Comment #25
xjmComment #26
xjmComment #27
BerdirIt'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.
Comment #28
alexpottThe 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 :(
Comment #29
yched CreditAttribution: yched commentedDunno 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 :-(
Comment #30
xjmI 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.
Comment #31
Berdir#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)
Comment #32
andypostAnother 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.Comment #33
jessebeach CreditAttribution: jessebeach commentedThis should be 50 chars instead of 64.
Comment #34
xjmI 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
orfield.instance
). Since the module name is already capped at 50, I think it's safe to make the second part (what's currently theconfig_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?
Comment #35
BerdirWe 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.
Comment #36
zserno CreditAttribution: zserno commentedRerolled patch from #27 in respect of comment #33 and using the 32 character limit. Might be missing some more core entity types though.
Comment #37
yched CreditAttribution: yched commentedWhile 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 ?
Comment #39
Berdir36: standardize-bundle-length-1709960-36.patch queued for re-testing.
Comment #41
dasrecht CreditAttribution: dasrecht commented36: standardize-bundle-length-1709960-36.patch queued for re-testing.
Comment #42
dasrecht CreditAttribution: dasrecht commentedI ran the tests in question locally and they worked well. Queued the patch for re-testing.
Comment #43
BerdirWhy 50?
Comment #44
zserno CreditAttribution: zserno commented@berdir As per #33. I think it's because module names can be 50 characters long.
Comment #46
zserno CreditAttribution: zserno commented@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:
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? :)
Comment #47
xjm#33 was incorrect; it should be 32 characters.
Comment #48
zserno CreditAttribution: zserno commentedDiscussed it with xjm on IRC:
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:
Comment #49
BerdirLet's discuss this in person, visit me in the Margrathea room.
Comment #50
zserno CreditAttribution: zserno commentedWe 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.
Comment #52
BerdirLooks good already, some feedback below, mostly minor things.
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 :)
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..."
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.
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.
The bundle needs a separate documentation block.
Comment #53
BerdirHere'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.
Comment #54
jessebeach CreditAttribution: jessebeach commentedComment #55
zserno CreditAttribution: zserno commentedComment #56
jessebeach CreditAttribution: jessebeach commentedAfter discussion with zserno and Berdir, we're making the following changes.
\Drupal\Core\Entity\EntityTypeIdLengthException
and\Drupal\Core\Entity\ConfigEntityIdLengthException
.\Drupal\Core\Entity\EntityMalformedException
inDrupal\Core\Entity::presave
with\Drupal\Core\Entity\ConfigEntityIdLengthException
\Drupal\Core\Entity\EntityMalformedException
inDrupal\Core\Entity::__construct()
withEntityTypeIdLengthException
.\Drupal\Core\Entity\EntityMalformedException
altogether if possible.Comment #58
zserno CreditAttribution: zserno commentedPosted a followup for comment bundle machine names: #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form.
Comment #59
tim.plunkettPlease put EntityTypeIdLengthException in Drupal\Core\Entity\Exception, and consider putting ConfigEntityIdLengthException in Drupal\Core\Config\Entity\Exception.
See #2228505: Clean up Entity exception classes
Comment #60
zserno CreditAttribution: zserno commentedReroll 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...
Comment #62
BerdirOh 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?
Comment #64
jessebeach CreditAttribution: jessebeach commentedIt 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.
Comment #65
yched CreditAttribution: yched commentedNo-one answered #37 ? How do we deal with length restrictions on data migrated from D7 where those restrictions didn't exist ?
Comment #66
Berdir@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.
Comment #67
jessebeach CreditAttribution: jessebeach commentedRight, 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:
Code review
Here is the migration followup #2229209: Restrict entity ID and bundle machine name string length to 32 characters, mentioned in #65/#66.
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.
Comment #68
BerdirThat 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 :)
Comment #69
andypostfor 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
Comment #70
Berdir64: standardize-bundle-length-1709960-64.patch queued for re-testing.
Comment #71
zserno CreditAttribution: zserno commented#2220757: Limit length of Config::$id to something <= 166 characters is just in, together with
\Drupal\Core\Entity\ConfigEntityIdLengthException
. Rerolling...Comment #73
zserno CreditAttribution: zserno commentedRemoved
\Drupal\Core\Entity\ConfigEntityIdLengthException
from patch.Comment #75
Berdir*grml* Unit tests *grml*
Comment #76
zserno CreditAttribution: zserno commentedReturned green, looking good to me, unassigning.
Comment #77
jhodgdonUmmmm... 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?
Comment #78
BerdirWe 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 :)
Comment #79
effulgentsia CreditAttribution: effulgentsia commentedWhy are we throwing a ConfigEntity exception from Entity? Should this move to ConfigEntityBase?
Comment #80
BerdirGood 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?
Comment #81
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #82
tim.plunkettNote that #2204697: Move getConfigPrefix() to ConfigEntityTypeInterface adds ConfigEntityTypeInterface
Comment #83
effulgentsia CreditAttribution: effulgentsia commentedIn 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.
Comment #84
alexpottI think this needs a change record since vid's in Drupal 7 could be longer than 32.
Comment #85
jessebeach CreditAttribution: jessebeach commentedI'm working on the change notice.
Comment #86
jessebeach CreditAttribution: jessebeach commentedChange notice is ready. https://drupal.org/node/2232665
Comment #87
jessebeach CreditAttribution: jessebeach commented75: standardize-bundle-length-1709960-75.patch queued for re-testing.
Comment #89
webchickCommitted and pushed to 8.x. Thanks!
I'm not sure if this actually can be backported, but moving it to 7.x nonetheless.
Comment #90
BerdirWe 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!
Comment #91
xjmI 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!
Comment #92
xjmMaybe 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).
Comment #94
bdone CreditAttribution: bdone commentedadded #2244775: Declare a maximum length for contact category machine name as a follow up that might have been missed here.
Comment #95
dqdFor 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.
Comment #96
BerdirNo, 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.
Comment #97
dqdThat'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).
Comment #98
BerdirNo, 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.
Comment #99
dqdHa. 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.
Comment #100
dqdWell, in this case it breaks the Drupal installation completely with no way out. This is indeed a hard fail...