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.

Tried replicating in #13 what step was I missing

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 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. And it overall does not look to respect best practices.

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.

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