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.

Rerolled the patch #18 against 10.3.

Fixes should be in an MR for tests to run and against 11.x

I have replicated the issue regarding the source field name length and applied the necessary fixes, which appear to resolve the issue. After reviewing the test cases, I identified several underlying problems, all of which have been addressed. Following these fixes, the functionality is now working correctly, and the source field name is properly truncated to a maximum of 32 characters. I have submitted a merge request for this change and am moving it to "Needs Review." Kindly review it at your convenience.

I have looked into how to convert the annotation for MediaSource to an attribute. However, I noticed that there is currently no predefined attribute available for MediaSource, unlike some other components like Action, which do have attributes defined.

Could you please guide me on whether we need to create a new attribute for MediaSource, similar to Action, before changing the plugin from annotation to attribute? Your guidance on this would be greatly appreciated.

Moving this to 'Needs Review' to get your help. Sorry for the inconvenience.

MediaSource already exists as an attribute, check the media mdoule.

Thank you for your help, I have verified that the MediaSource attribute has already been added.
my bad missed to check this. I have addressed the feedback accordingly and am moving this to "Needs Review."

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
/builds/issue/drupal-3276845/core/modules/field/src/Entity/FieldStorageConfig.php:357
/builds/issue/drupal-3276845/core/modules/field/src/Entity/FieldStorageConfig.php:329
/builds/issue/drupal-3276845/core/lib/Drupal/Core/Entity/EntityStorageBase.php:528
/builds/issue/drupal-3276845/core/lib/Drupal/Core/Entity/EntityStorageBase.php:483
/builds/issue/drupal-3276845/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:239
/builds/issue/drupal-3276845/core/lib/Drupal/Core/Entity/EntityBase.php:354
/builds/issue/drupal-3276845/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:617
/builds/issue/drupal-3276845/core/modules/media/tests/src/Kernel/MediaSourceTest.php:531
ERRORS!
Tests: 10, Assertions: 218, Errors: 1.
Exiting with EXIT_CODE=2

Test coverage appears to be there.

And all feedback appears to be addressed.

Left a question on the MR

Thank you, @larowlan, for the review and feedback. I am looking into this.

As per the MR feedback, I debugged and verified that the output of EntityTypeInterface::ID_MAX_LENGTH is correct and returns 32. This makes it a better approach compared to using a hard coded value. I have adjusted the logic accordingly based on these changes.
I also re-ran the tests, and everything is working as expected. Leaving this in Review for your feedback.

Hello,
Verified on Drupal 11,
MR applied successfully,
Not able to reproduce issue.
Attaching before and after screenshots.

Code can still be improved. Nits from #23 are still not fixed.

Thank you, @BramDriesen, for the review and feedback. I have updated the code according to the feedback provided by you. For the constant, I have removed the variable and directly used it in the checks. I re-ran the tests after these changes, and it is working as expected. Moving on for your review—please review it and let me know if anything else needs to be updated.

Looks ok to me. Still not easily readable though. Maybe converting '_' . $tries to "_$tries" helps, but I'll leave that in the middle for now.

Tests works as expected since the test only run failed.

larowlan → closed merge request !9932

Committed to 11.x and backported to 11.1.x - thanks!

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