MediaSourceBase::getSourceFieldName() doesn't check character max

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

On Drupal 10 by cloning the image source file and using the test case annotation

/**
 * Provides test media source.
 *
 * @MediaSource(
 *   id = "test_source_with_a_really_long_name",
 *   label = @Translation("Test source with a really long name"),
 *   description = @Translation("Test source with a really long name."),
 *   allowed_field_types = {"string"},
 * )
 */
class TestSourceWithAReallyLongName extends File {

I would expect to get the error because the id test_source_with_a_really_long_name is too long but it passes just fine.

Is there a missing step?

Add patch against #9 in 10.1 version.

Hiding patch #14 as it's incomplete. You omitted the /core/modules/media/tests/modules/media_test_source/src/Plugin/media/Source/TestSourceWithAReallyLongName.php

@Deshna Chauhan Please stop uploading incomplete and broken re-roll patches without an interdiff.

@Deshna Chauhan, I am removing credit for the unhelpful patch per How is credit granted for Drupal core issues → .

Attached is rerolled patch for Drupal 10.1. Also attached is a failing patch to demonstrate the bug via test coverage.

The output I'm seeing locally for the failing patch is provided below:

$ ./vendor/phpunit/phpunit/phpunit -c core/ core/modules/media/tests/src/Kernel/MediaSourceTest.php --verbose
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.8
Configuration: /var/www/html/core/phpunit.xml.dist

Testing Drupal\Tests\media\Kernel\MediaSourceTest
......E...                                                        10 / 10 (100%)

Time: 00:32.158, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\media\Kernel\MediaSourceTest::testSourceFieldCreation
Drupal\Core\Field\FieldException: Attempt to create a field storage with an name longer than 32 characters: field_media_test_source_with_a_really_long_name

/var/www/html/core/modules/field/src/Entity/FieldStorageConfig.php:331
/var/www/html/core/modules/field/src/Entity/FieldStorageConfig.php:302
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:528
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:483
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:257
/var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:339
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:608
/var/www/html/core/modules/media/tests/src/Kernel/MediaSourceTest.php:523
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

ERRORS!
Tests: 10, Assertions: 218, Errors: 1.

This bug popped up again yesterday over in the oEmbed Provider module's issue queue: 🐛 Attempt to create a field storage with an name longer than 32 characters Closed: duplicate . Folks are definitely still running into it.

Tried replicating in #13 what step was I missing

The last submitted patch, 18: drupal-long_media_source_name-3276845-18-FAIL.patch, failed testing. View results →

@smustgrave - I'm not 100% clear on steps followed in #13.

If you take a look at the test coverage provided by the patch, you'll see two steps:

Step 1: Create a Media Source plugin:

namespace Drupal\media_test_source\Plugin\media\Source;

/**
 * Provides test media source.
 *
 * @MediaSource(
 *   id = "test_source_with_a_really_long_name",
 *   label = @Translation("Test source with a really long name"),
 *   description = @Translation("Test source with a really long name."),
 *   allowed_field_types = {"string"},
 * )
 */
class TestSourceWithAReallyLongName extends Test {

}

Step 2: Create a media type using said media source as its source:

    // Test a source with a long machine name.
    $type = MediaType::create([
      'id' => 'test_type_fail',
      'label' => 'Test type - Fail',
      'source' => 'test_source_with_a_really_long_name',
    ]);
    $type->save();

    /** @var \Drupal\field\Entity\FieldConfig $field */
    $field = $type->getSource()->createSourceField($type);
    $field->save();
    /** @var \Drupal\field\Entity\FieldStorageConfig $field_storage */
    $field_storage = $field->getFieldStorageDefinition();
    $field_storage->save(); // <-- THIS IS WHERE THE FAILING PATCH FAILED.

Failed test error:

1) Drupal\Tests\media\Kernel\MediaSourceTest::testSourceFieldCreation
Drupal\Core\Field\FieldException: Attempt to create a field storage with an name longer than 32 characters: field_media_test_source_with_a_really_long_name

A few small code nits. But I don't think that this should hold this back :)

1. +++ b/core/modules/media/src/MediaSourceBase.php
   @@ -309,10 +309,18 @@ protected function getSourceFieldName() {
   +        $suffix = '_' . $tries;
   +        $id .= $suffix;
   

   Maybe a small nit, but this could be converted to a single line of code.

2. +++ b/core/modules/media/src/MediaSourceBase.php
   @@ -309,10 +309,18 @@ protected function getSourceFieldName() {
   +          $suffix_character_count = strlen($suffix);
   +          $id = substr($base_id, 0, (32 - $suffix_character_count)) . $suffix;
   

   Don't think it's needed to cast the strlen to a variable.

The last submitted patch, 18: drupal-long_media_source_name-3276845-18.patch, failed testing. View results →

The failing test was unrelated and an intermittent FunctionalJavascript failure. I reran tests, and they passed. Setting back to RTBC.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.