Fix \Drupal\taxonomy\Entity\Term::getName() to conform to the interface

Here is a patch :)

Tested the patch here and it worked for me, and the code follows the "Proposed resolution".
Steps:

• Opened an Drupal environment with PHP 8.1
• Went to /admin/structure/taxonomy/manage/tags/overview/fields, added an image field with all default configurations
• Went to /admin/structure/taxonomy/manage/tags/add to add a tag
• Before applying the patch:
• When i add an image before setting the term name, an message shows up (same error of the IS)
• After applying the patch:
• I can add an image without getting the message

Changing this to RTBC.

Retested patch #3 on Drupal 9.5.x. It's green so back to RTBC as per comment #4 and #7.

Given we obviously have no test coverage of this I feel it is worth adding some.

We should be able to use \Drupal\Tests\taxonomy\Functional\TaxonomyImageTest as the basis for a test - we could make it a FunctionalJavascript test and go from there.

That test got removed in #3305410: remove TaxonomyImageTest -- someone working on this will need to retrieve the test class from git history (removal commit is 0cf5ca1 on 10.1.x).

New patch including tests.

Instead of replicating the scenario in a new FunctionJavascript test, it makes the form validation less restrictive to make the bug visible.

Confirmed the issue described and the patch #14 solves it.

And the failing test shows the issue is being solved.

Please review.

Removing credit for #18
As there was no interdiff
No explanation of change
And reroll removed most of the changes.

Failure in #14 seems random. Retesting but going to mark too.

+++ b/core/modules/taxonomy/tests/modules/taxonomy_test/taxonomy_test.module
@@ -45,4 +45,8 @@ function taxonomy_test_form_taxonomy_term_form_alter(&$form, FormStateInterface
+    $form['name']['widget'][0]['value']['#required'] = FALSE;

I don't think allowing to save an invalid term is best way to test this because it's certainly not a behavior we want to have to support.

If the module wanted to ensure that the entity is only saved if it's valid, it should generate the title before saving it. Looks like it's currently done in hook_entity_insert() which means it's done after it has been saved. 🤷‍♂️

Here's a test that uses the steps from the issue summary.

Addressed #25 👍

Now that I'm looking at this again, it looks like \Drupal\taxonomy\TermInterface::getName is supposed to always return string but \Drupal\Core\Entity\EntityInterface::label might return string|\Drupal\Core\StringTranslation\TranslatableMarkup|null. I think the right fix would be for \Drupal\taxonomy\Entity\Term::getName to return empty string when \Drupal\Core\Entity\ContentEntityBase::label returns null.

Makes sense to me and can see the test-only patch fail in 23.

Ran locally too
Exception : Deprecated function: trim(): Passing null to parameter #1 ($string) of type string is deprecated
Drupal\taxonomy\TermForm->buildEntity()() (Line: 152)

The issue summary is clear and there is a proposed resolution. I read the comments and all questions have been answered. However, I notice that the last comment has changed the solution making the proposed resolution incorrect. And the title isn't quite right any more.

I looked at the patch and noticed a doc block missing, the use statements were not sorted, $module should have an @inheritdoc. I have made a patch for those.

I have updated the proposed resolution and tried for an better title.

Got hit by this issue on 9.5.x . But patch from #29 did not fix it.

Seems Patch from #29 has random failure, rerunning the tests.

Seems changes in #29 are good/all green

@Bhanu951 can you post the error/steps you're taking to get this issue?

Retesting.

Moved #29 into MR. Looks like the CI is passing so moving back to RTBC.

@lauriii Please remember to hide patch files from the IS when opening an MR. Thanks!

I queued the test-only job. Results:

) Drupal\Tests\block\Kernel\Migrate\d6\MigrateBlockTest::testBlockMigration
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'Schema errors for block.block.aggregator with the following errors: block.block.aggregator:settings.block_count missing schema, block.block.aggregator:settings.feed missing schema, 0 [settings.block_count] 'block_count' is not a supported key., 1 [settings.feed] 'feed' is not a supported key.'
+'Schema errors for block.block.aggregator with the following errors: block.block.aggregator:settings.block_count missing schema, block.block.aggregator:settings.feed missing schema, 0 [settings] 'block_count' is not a supported key., 1 [settings] 'feed' is not a supported key.'
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/builds/project/drupal/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php:313
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 1, Assertions: 123, Failures: 1.

That is not the failure I expected.

https://www.drupal.org/pift-ci-job/2723504 documents the expected test-only result, so the issue with the test-only job is not blocking. I reached out to the DA and committer team about it.

I fixed a few nitpicks. Apparently the weird failure from the test-only job might be due to the MR not being rebased, so also fixed that. (Adding credit for @fjgarlin for identifying that issue.)

Meanwhile, saving issue credits.

The rebase did yield the correct test-only result:

There was 1 error:
1) Drupal\Tests\taxonomy\Functional\TaxonomyImageTest::testTaxonomyImageUpload
Exception: Deprecated function: trim(): Passing null to parameter #1 ($string) of type string is deprecated
Drupal\taxonomy\TermForm->buildEntity()() (Line: 158)
/builds/project/drupal/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:55

I could not think of any way that no longer returning an invalid NULL from this would be particularly disruptive, so I committed this to 11.x, 10.2.x, and 10.1.x. Thanks!