Limit length of Config::$id to something <= 166 characters

The problem here and in other related issues (like comment bundle length) are composite ID's where the ID is not the same as the machine name entered in the UI.

The ID of a field instance is created based on 3 parts: $this->entity_type . '.' . $this->bundle . '.' . $this->field->name

If you want to limit the ID of config entities, and have an entity type and a bundle on that with each 25 characters, it's impossible to create fields for that.

Bundles are usually fairly short, but comment is an interesting example where the bundle consists of the field id of it's comment field.

So for example he comment_body field instance on nodes has an ID like this: "comment.node__comment.comment_body". Those are all very short, so it's only 34 characters, but let's assume that we're going to add a comment field named comment to a commerce_order_line_item entity. That's already 54 and too long. We could shorten the field name to just body, because we no longer need to separate it from the node body field by name, which would work for this, but with the default field prefix, you get "comment.commerce_order_line_item__comment.field_", which is 48 characters, which leaves users exactly 2 characters to name their fields on comments on commerce order line items ;)

Re #1 I don't think we should apply a general rule - also I took this issue to be proposing that in the case of fields and their instances it'd be the $this->field->name part that would be limited. As this is already limited to 32 nothing needs to be done for fields / field instances.

Isn't this a duplicate of #1191434: Standardize entity type is a maximum of 32 characters (not 64 or 128)? The point of declaring it at the entity level is that it then propagates down to all child classes, including config entities and fields.

It's related but not the same.

*Some* config entities have ID's that relate to entity types or bundles, but most don't. This is about a default verification/validation for everyhting.. views, actions, blocks... as I understand it?

#2: Hm, maybe, but that can't be enforced globally/by default then as we don't know which property we need to check (instead of just id()) and it would theoretically mean that a field instance ID could be 132 characters (or at least 3 * 32, if we limit bundle and entity_type to 32 as proposed by @xjm there).

But... Looking at #2120003: [META] Create sensible limits for the maximum length of configuration object filename components, the point is to limit **each part (between the "."s) of the configuration file name** to 50 characters, not the whole thing.

So in the case of a field instance config or field config, you have:
(field config): field .field .node .field_my_entered_machine_name
(instance config): field .instance .node .article .field_my_entered_machine_name

Each part of this is limited to 50 characters:
- The module name "field", OK.
- The config ID, "field" or "instance", OK.
- The entity type, "node", OK.
- The entity bundle, "article", OK.
- The field's machine name, which we need to limit to 50 characters (including the "field_" which the Field UI module puts in there).

Comment #1 here seems to think that the whole thing node.body or node.article.body is limited to 50 characters, but that is not the proposal. The proposal is (unless I am mistaken) that the machine name itself -- the part that the user enters + the standard field_ prefix from Field UI, needs to be 50 characters or less. This doesn't sound like a huge problem?

Ok, yes, that seems fine, I misunderstood that a bit.

We have a different understanding of name component I guess. For me, the config entity ID is a single component, because it can not be separated further. That it uses "." is basically an implementation detail, it could also be a different character. And nothing is stopping me from creating an entity type that has 4 parts that make up the ID?

So for the way I understand it, we should say that a config entity ID must not be longer than 149 characters (shouldn't we include the "." in our calculations too, 50 always includes the "." as well?) , and is free to split that up.

Which means that for a field_instance, we know that with with the decision in #1709960: declare a maximum length for entity and bundle machine names to limit entity type and bundle to 32, entity_type.bundle. can have a maximal length of 32 + 1 + 32 + 1 => 66, Which means that field names could be up to 84 characters long and we'd still be good. And since they are already limited to 32 characters, we're all set and someone could even come up with a config entity that is entity_type.bundle.field_name.whatever ;)

And a view could have a machine name that is up to 149 characters long.

Re. #4, right, sorry, I was confusing two issues. So this issue is about specifying a maximum length for Config::$id, e.g. in:
views.view.frontpage.yml
field.instance.node.article.default.yml
entity.form_display.node.article.default.yml
The following parts would be the part capped for this issue in each case:
frontpage
node.article.default
node.article.default

The goal of #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term) is to change it so that the entity type ID becomes a compound of module_name.what_we_currently_call_config_prefix_in_the_entity_but_not_the_method_name, e.g. views.view or node.article. If module_name is capped at 50 (as it is in HEAD), then we would need the combination of what_we_currently_call_config_prefix_in_the_entity_but_not_the_method_name and the ID to add up to 200 characters max. If we make what_we_currently_call_config_prefix_in_the_entity_but_not_the_method_name 32 characters as proposed in #2120003: [META] Create sensible limits for the maximum length of configuration object filename components, then that would leave 168 166 characters for the rest. (Edit: taking into account the dots.)

So if I understand correctly, a config name can be up to 250 characters, of which 50 characters is the module name, 32 characters is the prefix, and the rest (up to 166 characters) is the ID (sometimes called machine name).

I'm new to this so this could be the wrong place to put this, but here's a check that raises an exception before saving the config entity if the limit is exceeded.

Also trying to add a test for this...

Testing for Config entity "prefix" length (83) and "id" length (166) in ConfigStorageController::save()

This patch includes the changes from 2220757-15.patch

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -80,6 +80,21 @@
      /**
   +   * The maximum length of a configuration object ID.
   +   *
   +   * Many filesystems (including HFS, NTFS, and ext4) have a maximum file name
   +   * length of 255 characters. To ensure that no configuration objects
   +   * incompatible with this limitation are created, we enforce a maximum name
   +   * length of 250 characters (leaving 5 characters for the file extension).
   +   *
   +   * @see http://en.wikipedia.org/wiki/Comparison_of_file_systems
   +   *
   +   * 50 characters of the 250 is reserved for the module name,
   +   * 32 characters for the config prefix, that leaves 166 characters for the ID length.
   +   */
   +  const MAX_ID_LENGTH = 166;
   

   Not sure if we want to copy the documentation from ConfigBase. Maybe just add a @see to it?

   The description should also not be wider than 80 characters.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -259,6 +274,13 @@ public function toArray() {
   +    if (strlen($this->id()) > self::MAX_ID_LENGTH) {
   +      throw new \Drupal\Core\Config\ConfigNameException(String::format('Config entity machine name (id) @name exceeds maximum allowed length of @length characters.', array(
   +        '@name' => $this->id(),
   +        '@length' => self::MAX_ID_LENGTH,
   +      )));
   +    }
   

   Just discussed this with @alexpott and we wondering who and where this check should live.

   We thought that it makes more sense that ConfigStorageController::save() does this because this is a storage limitation. For example, @timplunkett right now is working on a key value storage which has only allows 128 character long ID's, so he needs a different check. And someone could experiment with a different storage controller in contrib that allows longer ID's.

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
   @@ -288,6 +291,15 @@ public function delete(array $entities) {
   +    //check config entity prefix length, should not be > 83 (MAX_PREFIX_LENGTH)
   +    //NOTE: $this->getConfigPrefix() returns the content entity prefix plus a '.'
   +    if (strlen($prefix) > self::MAX_PREFIX_LENGTH+1) {
   +      throw new ConfigNameException(String::format('Config entity prefix ({extension}.{identifier}) @name exceeds maximum allowed length of @length characters.', array(
   +       '@name' => rtrim($prefix, '.'),
   +       '@length' => self::MAX_PREFIX_LENGTH,
   +     )));
   +    }
   

   We have a separate issue for this, this shouldn't be checked here.

4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
   @@ -315,6 +327,14 @@ public function save(EntityInterface $entity) {
    
   +    //check config entity id length, should not be > 166 (ConfigEntityBase::MAX_ID_LENGTH)
   +   if (strlen($entity->{$this->idKey}) > ConfigEntityBase::MAX_ID_LENGTH) {
   +     throw new ConfigNameException(String::format('Config entity machine name (id) @name exceeds maximum allowed length of @length characters.', array(
   +       '@name' => $entity->{$this->idKey},
   +       '@length' => ConfigEntityBase::MAX_ID_LENGTH,
   +     )));
   +   }
   

   Actually we have the check here as well? Why both? :)

   Comment nitpick: Should have a space after //, start with an uppercase character, not be longer than 180 characters and end with a ".".

We thought that it makes more sense that ConfigStorageController::save() does this because this is a storage limitation.

Actually we have the check here as well? Why both? :)

config entity id length checking in ConfigStorageController::save() only

For example, @timplunkett right now is working on a key value storage which has only allows 128 character long ID's, so he needs a different check. And someone could experiment with a different storage controller in contrib that allows longer ID's.

checking is based on an optional constant MAX_ID_LENGTH introduced to the storage service (see Drupal\Core\Config\FileStorage)

Comments / descriptions updated

Sorry, maybe I wasn't clear. The constant has to live in the config entity storage controller, *not* in the config storage. I was talking about a different entity storage controller, not a different config storage. So the dynamic stuff there isn't necessary, just use your own constant.

This also needs a test, you should probably extend ConfigEntityTest::testCRUD().

Constant moved back and value corrected

As for the tests, I'll try to write some as soon as I learn how :)

I'm in the Margrathea room in Szeged if you're here, happy to show you how to get started with tests.

First attempt at tests.

- Updated the patch with the recent class name changes.
- Changed the comments to the proposed ones, with some modifications.
- Using the actual constant from the class rather than fixed lengths.

Thanks @Karol Haltenberger! Can you be sure to post interdiffs when you create new patches? See: https://drupal.org/documentation/git/interdiff

We just discussed that this should use the exception class from #2220739: Custom block types can be 64 characters long but the custom_block only supports 32. (ConfigEntityIdLengthException)

Note that #1709960: declare a maximum length for entity and bundle machine names will add and use the same exception class, but we can continue by adding it to both issues, then we can simply re-roll the other one once one is in. No need to postpone issues.

I'll reroll to add it, plus clean up the following nitpicky things. :)

1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
   @@ -142,6 +145,56 @@ function testCRUD() {
   +    // Verify that a configuration entity can be saved with
   +    // an ID of maximum allowed length, but not longer.
   

   Wrapping a bit early.

2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
   @@ -142,6 +145,56 @@ function testCRUD() {
   +    // Test with a short ID
   +    // By default randomName() generates a 8 character long name
   

   Needs periods at the end of sentences. Also could be wrapped into one comment.

3. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
   @@ -142,6 +145,56 @@ function testCRUD() {
   +    $config_test_idlen = entity_create('config_test', array(
   

   Usually we don't use abbreviations like "idlen". Also, this variable does not store an ID length, it stores a configuration entity object, so we could call it something like $test_config_entity.

4. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
   @@ -142,6 +145,56 @@ function testCRUD() {
   +    // Test with an ID of maximum allowed length
   ...
   +    // Test with an ID exeeding the maximum allowed length
   

   Also need periods. :)

Attached uses the new exception class and cleans up the things in #30. I also tested it manually!

Installing with default config with a too-long ID

Trying to synch config with a too-long ID

Let's actually attach the patch, eh?

@berdir said he was +1 with the current version of the patch, and suggested we let @jessebeach take a final look at it as well.

I found another couple non-blocking coding standards cleanups.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityIdLengthException.php
   @@ -0,0 +1,15 @@
   + * Exception thrown when a configuration entity ID is too long.
   

   Should start with a verb.

2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
   @@ -142,6 +145,56 @@ function testCRUD() {
   +      'id' => $this->randomName(ConfigEntityStorage::MAX_ID_LENGTH+1),
   

   There should be spaces around the +.

3. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
   @@ -142,6 +145,56 @@ function testCRUD() {
   +      $this->fail(String::format("config_test entity with ID length @len, exceeding the maximum allowed length of @max saved successfully", array(
   ...
   +      $this->pass(String::format("config_test entity with ID length @len, exceeding the maximum allowed length of @max failed to save", array(
   

   Grammatically, there should either be a comma after @max, or no comma after @len.

Patched patch inc.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -38,6 +39,13 @@
      /**
   +   * The maximum length of a configuration entity ID.
   +   *
   +   * @see \Drupal\Core\ConfigBase::MAX_NAME_LENGTH
   +   */
   +  const MAX_ID_LENGTH = 166;
   

   In the config prefix issue (ConfigEntityType::PREFIX_LENGTH), we added an explanation why the prefix is limited to 83, we should probably do the inverse here.

2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
   @@ -142,6 +145,56 @@ function testCRUD() {
   +    // Test with a short ID. By default randomName() generates an 8 character
   +    // name.
   +    $id_length_config_test = entity_create('config_test', array(
   +      'id' => $this->randomName(),
   

   Would be easier to just make 8 or something else explicit instead of a long explanation....

According to https://drupal.org/node/1709960#comment-8631877, please put ConfigEntityIdLengthException in Drupal\Core\Config\Entity\Exception. See latest patch there as an example.

Thanks jessebeach. Rerolled according to feedback at #38 and #39.

+1, all that looks good to me.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -38,6 +39,19 @@
   /**
+   * Length limit of the configuration entity ID.
+   *
+   * Most file systems limit a file name's length to 255 characters. In
+   * order to leave sufficient characters to construct a configuration prefix,
+   * the configuration entity ID is limited to 166 characters which
+   * leaves 83 characters for the configuration prefix. 5 characters are
+   * reserved for the file extension.
+   *
+   * @see \Drupal\Core\Config\ConfigBase::MAX_NAME_LENGTH
+   */
+  const MAX_ID_LENGTH = 166;

That is the weirdest, most arbitrary limit for something I've ever seen, and leaves another weird, arbitrary limit (83) to work with. And unfortunately, the docs that are here do not in any way help me to understand how we arrived at that limit, and why we shouldn't change that some day in the future to something less weird and arbitrary.

Asking around in IRC, xjm said that this was arrived at because 50 was reserved for the module name, 32 for the entity ID, plus a dot. I think we could really do to explain this to people because I can't be the only one who sees this and goes "Huh?!"

xjm spun off a sub-issue for this, as well as to unify the docs of these constants more generally: #2229931: Improve documentation related to config object filename component limits

The only other very minor thing here is we don't generally abbreviate things so all the places that are @len should be @length. Fixed on commit.

Committed and pushed to 8.x. Thanks!