Problem/Motivation

Part of #2120003: [META] Create sensible limits for the maximum length of configuration object filename components

The config entity type config_prefix property may form a part of a config entity's file name -- the yml file that represents an instance of the entity. The file name length is limited to 255 characters by most operating systems. To be safe, we limit the name to 250 characters and leave 5 for a file extension. The file name is potentially composed of 5 pieces. Each part may therefore only consume 20% of the available characters, so the config entity type config_prefix can therefore only be a maximum of 50 characters long.

Here is an example of an config entity type config_prefix in Core using the EntityFormDisplay config entity.

/**
 * @ConfigEntityType(
 *   id = "entity_form_display",
 *   label = @Translation("Entity form display"),
 *   config_prefix = "form_display",
 *   entity_keys = {
 *     "id" = "id",
 *     "status" = "status"
 *   }
 * )
 */

Proposed resolution

Config entity types with a config_prefix greater than 50 characters must:

  1. Fail installation
  2. Prevent the owning module from installing as well.

This will keep contrib authors from creating config entity types with config_prefix greater than 50 characters and discourage adding them to D.O.

User interface changes

None.

API changes

Perhaps new methods.

Original report by @jessebeach

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

I think our goal is ultimately to get rid of config_prefix as a separate concept entirely through #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term), so pursuing that goal foremost might be a better way to tackle this.

jessebeach’s picture

jessebeach’s picture

Title: Limit config_prefix values to 50 characters » Limit $extension.$(config_prefix/entity_id) combo value to 83 characters (82 plus a '.') as part of a 250 character limit on config entity file names
Status: Closed (duplicate) » Active

Reopening this as a replacement for #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term), which is now postponed to 9.x.

By limiting the combo of extension name and config_prefix/entity_id to 83 characters, we leave 166 characters (combining to a maximum of 250 characters) for a config entity file name.

The 83 character limit is imposed as a combination in order to allow for short module names and long config_prefixes/entity_ids or the reverse.

gremy’s picture

Assigned: Unassigned » gremy
gremy’s picture

Status: Active » Needs review
FileSize
4.27 KB

Added a new exception, ConfigPrefixLengthException, that is thrown in getConfigPrefix() if the output length is more than 83 characters.

gremy’s picture

Rerolled patch: changed strlen to drupal_strlen

dawehner’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
    @@ -0,0 +1,70 @@
    +  /**
    +   * The entity under test.
    +   *
    +   * @var \Drupal\Core\Config\Entity\ConfigEntityBase|\PHPUnit_Framework_MockObject_MockObject
    +   */
    +  protected $entity;
    

    I think you can just use $entity_type = new ConfigEntityType ... in the test functions and get rid of this

  2. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
    @@ -0,0 +1,70 @@
    +   * @expectedException \Drupal\Core\Config\ConfigPrefixLengthException
    

    Note: There is also @expectedExceptionMessage to check the message. Your message is not absolute trivial, so it would be nice to also test that

  3. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
    @@ -0,0 +1,70 @@
    +
    

    Nitpick: here are two extra lines.

Status: Needs review » Needs work

The last submitted patch, 6: 2220749-6-limit-config-prefix-lenth.patch, failed testing.

gremy’s picture

Status: Needs work » Needs review
FileSize
4.54 KB

Rerolled patch based on feedback.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigPrefixLengthException.php
    @@ -0,0 +1,13 @@
    +* Exception thrown when the config prefix length is exceeded
    

    Missing . at the end I think?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -35,17 +37,24 @@ public function getControllerClasses() {
         // Ensure that all configuration entities are prefixed by the module that
         // provides the configuration entity type. This ensures that default
         // configuration will be created as expected during module install and
         // dependencies can be calculated without the modules that provide the
         // entity types being installed.
    -    return $this->provider . '.' . $config_prefix;
    +    return $config_prefix;
    

    The comment here is now misplaced, as the action that it describes has been moved up. The comment should probably move to the beginning of the method.

  3. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
    @@ -0,0 +1,62 @@
    +  public function testConfigPrefixLengthNormal() {
    

    There's one additional combination of what that function does, and that's specifying a provider + id without config_prefix. Should we add a test for that as well while we're at it?

gremy’s picture

1. No missing . at the end. That should be in the comment and it is there.
2. Moved comment
3. Not the scope of this issue. The tests in this issue check if based on the length of the config prefix we the exception is thrown.

gremy’s picture

Status: Needs review » Needs work

The last submitted patch, 12: 2220749-10-limit-config-prefix-lenth.patch, failed testing.

Berdir’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
    @@ -0,0 +1,62 @@
    +   * Test that we get an exception if getConfigPrefix() returns a ConfigPrefixLengthException
    

    Well, if it throws (not returns) an exception, then obviously we get one. This should say something like "Tests that we get an exception when the config prefix is too long", and the one below "... when the config prefix is not too long". Explain the why.

    Also, both *are* missing a . at the end of the line. As is the exception description.

  2. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
    @@ -0,0 +1,62 @@
    +      'provider' => 'extra_long_provider_name', // 24 characters
    +      'config_prefix' => 'long_random_configiguration_prefix_so_that_go_over_the_limit', // 60 characters
    +    ));
    +    $this->assertEmpty($config_entity->getConfigPrefix()); // this should have 60 + 25 + 1 = 84 characters
    ...
    +      'provider' => 'extra_long_provider_name', // 24 characters
    +      'config_prefix' => 'random_configiguration_prefix_that_exactly_at_limit_123456', // 58 characters
    +    );
    +    $config_entity = new ConfigEntityType($entity_data);
    +    $expected_prefix = $entity_data['provider'] . '.' . $entity_data['config_prefix'];
    +    $this->assertEquals($expected_prefix, $config_entity->getConfigPrefix()); // this should have 58 + 25 + 1 = 83 characters
    

    One of those descriptions is lying, 58 / 60 but 84/83 ;)

    Also, those comments need to be formatted according to our coding standards, which means on their own line, start with a upper case character should be a full sentence and ending with a ".".

    I would write this as a single sentence at the top of the method: "// A config entity with a provider length of N and config_prefix length of N (+1 for the .) results in a config length of N, which is too long."

    Make sure to wrap it 80 characters.

Berdir’s picture

Note that the test fails were a bot problem, you can ignore them.

Also, I would still argue that testing it with id makes sense because that could be too long too. While that does not matter for the current implementation, it could also be written like differently and we should be making sure every possible combination is verified.

gremy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: 2220749-10-limit-config-prefix-lenth.patch, failed testing.

gremy’s picture

Re-rolled patch based on feedback from Berdir

gremy’s picture

gremy’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work

Thanks, almost there :)

  1. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
    @@ -0,0 +1,100 @@
    +      'provider' => 'extra_long_provider_name',
    +      'id' => 'long_random_entity_id_so_that_we_will_go_over_the_limit12345',
    

    Wondering if we want to use $his->randomName(24) and so on for those strings, would in this case be more self-explaining than coming up with strings of that length ourself?

  2. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
    @@ -0,0 +1,100 @@
    +  public function testConfigPrefixLengthWithPrefixNormal() {
    

    Maybe "Valid" instead of "Normal"?

  3. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
    @@ -0,0 +1,100 @@
    +   * Test that we do not get a any exceptionswhen the config prefix is not too long.
    ...
    +    // A config entity with an provider length of 24 and id length of 58
    +    // (+1 for the .) results in a config length of 83, which is right at the limit.
    +    $entity_data = array(
    

    Comments look fine now, only problem is that some of them are over 80 characters wide, which they shouldn't.

    Not sure what to about the method description, maybe rewrite it a bit to something like "Tests that a valid config prefix does not throw an exception"?

    Also, the method descriptions for the ID methods still mention "config prefix".

gremy’s picture

1. At first I used $this->randomName(), but in order to be able to use @expectedExceptionMessage I switched to using made-up strings. I could however use it for 2 out of the 4 tests.
2. Sounds better with Valid ... I'll do that
3. I'll see what I can do

gremy’s picture

Status: Needs work » Needs review
FileSize
6.2 KB
Berdir’s picture

Thanks, made some small changes to the comments and switched to 59 instead of 60 so that we actually test the exact number we're checking.

This looks good to me, just needs an RTBC I think :)

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thank you tests look fine code change make scene RTBC.

jessebeach’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
@@ -34,18 +36,25 @@ public function getControllerClasses() {
+    if (strlen($config_prefix) > 83) {
+      throw new ConfigPrefixLengthException(String::format('The @config_prefix config_prefix length is larger than the maximum limit of @char_count characters', array(
+        '@config_prefix' => $config_prefix,
+        '@char_count' => 83,
+      )));

We should declare a constant for this limit rather than hard-coding 83.

The error message is misleading. It's not that config_prefix is too long at more than 83 characters; the 'configuration file name prefix' is too long. We should express the difference of provider name length and the config_prefix/id name length as the limit. Maybe something like:

'The configuration file name prefix @config_prefix exceeds the maximum character limit of @max_char'

Berdir’s picture

Ok, improved the exception message and added the constant.

jessebeach’s picture

I reviewed with Berdir in person. We're good to re-RTBC when it comes back green.

This issue is enough of an 8.x-8.x change that we need a change notice. I've started that here: https://drupal.org/node/2227275

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

Changes made. Tests passed. Change record is ready. Back to RTBC.

xjm’s picture

Assigned: gremy » alexpott

Let's have @alexpott take a look at this one.

alexpott’s picture

Assigned: alexpott » Unassigned
Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -15,6 +17,13 @@
    +   * Length limit of the config prefix returned by getConfigPrefix().
    +   *
    +   * @see \Drupal\Core\Config\ConfigBase::MAX_NAME_LENGTH
    +   */
    +  const PREFIX_LENGTH = 83;
    

    It would nice to document why we've chosen 83 here.

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -34,18 +43,25 @@ public function getControllerClasses() {
    +      throw new ConfigPrefixLengthException(String::format('The configuration file name prefix @config_prefix exceeds the maximum character limit of @max_char', array(
    

    Missing full stop.

alexpott’s picture

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
@@ -0,0 +1,104 @@
+  /**
+   * Tests that a valid config prefix does not throw an exception.
+   *
+   * @covers ::getConfigPrefix()
+   */
+  public function testConfigPrefixLengthWithPrefixValid() {
+    // A config entity with a provider length of 24 and config_prefix length of
+    // 58 (+1 for the .) results in a config length of 83, which is right at the
+    // limit.
+    $entity_data = array(
+      'provider' => $this->randomName(24),
+      'config_prefix' => $this->randomName(58),
+    );
+    $config_entity = new ConfigEntityType($entity_data);
+    $expected_prefix = $entity_data['provider'] . '.' . $entity_data['config_prefix'];
+    $this->assertEquals($expected_prefix, $config_entity->getConfigPrefix());
+  }
+
+  /**
+   * Tests that a valid config prefix does not throw an exception.
+   *
+   * @covers ::getConfigPrefix()
+   */
+  public function testConfigPrefixLengthWithIdValid() {
+    // A config entity with an provider length of 24 and id length of 58
+    // (+1 for the .) results in a config length of 83, which is right at the
+    // limit.
+    $entity_data = array(
+      'provider' => $this->randomName(24),
+      'id' => $this->randomName(58),
+    );
+    $config_entity = new ConfigEntityType($entity_data);
+    $expected_prefix = $entity_data['provider'] . '.' . $entity_data['id'];
+    $this->assertEquals($expected_prefix, $config_entity->getConfigPrefix());
+  }

These tests could be combined and use a data provider.

Berdir’s picture

Short explanation of what we discussed/I suggest:

For #31.1: Just add a sentence that says, limit prefix to 83 characters so that we have 166 left for the configuration entity ID. That's the only reason we have now, as you're free to split it between module and prefix.

#32:We could even make all 4 test methods use a provider by using setExpectedException(). *That* would actually allow us to use randomName() for those tests as well, because then we can dynamically build th exception message. Will avoid those very long strings and be more self-explaining I think. Look at other tests that have @dataProvider.

gremy’s picture

Rerolled patch based on feedback.

gremy’s picture

jessebeach’s picture

Assigned: Unassigned » jessebeach
Status: Needs review » Needs work
jessebeach’s picture

Status: Needs work » Needs review
FileSize
3.59 KB
7.82 KB

Just a few comment tweaks. No code changes. I reviewed the changes with Berdir in person and verified the test cases.

alexpott's #32 and Berdir's #33 comments have been addressed.

Back to RTBC when it comes back green.

jessebeach’s picture

Assigned: jessebeach » alexpott
Status: Needs review » Reviewed & tested by the community

This was green before. I just made comment edits. Back to RTBC.

Assigning to alexpott to put this on his radar.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 361ddb1 and pushed to 8.x. Thanks!

  • Commit 361ddb1 on 8.x by alexpott:
    Issue #2220749 by gremy, Berdir, jessebeach: Limit $extension.$(...
xjm’s picture

Component: base system » configuration entity system

Retroactively fixing the component.

Status: Fixed » Closed (fixed)

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