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

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.

The last submitted patch, 14: 3277238-14-test-only.patch, failed testing. View results →

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

And the failing test shows the issue is being solved.

The last submitted patch, 14: 3277238-14.patch, failed testing. View results →

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.

I'm unsure, I agree it's somewhat uncommon, but that's already a fairly common scenario that we do "support", the way it works is that you then generate a node title/term name based on some other fields, for example with https://www.drupal.org/project/auto_entitylabel → . Typically you hide the field then though (and can do that with nodes through the UI) but that's a minor difference. The same would indeed also happen there.

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.

The last submitted patch, 23: 3277238-23-test-only.patch, failed testing. View results →

+++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyImageTest.php
@@ -0,0 +1,95 @@
+      'name[0][value]' => $this->randomMachineName(),
+      'field_test[0][alt]' => $this->randomMachineName(),
+    ];
+    $this->submitForm($edit, 'Save');
+    $terms = \Drupal::entityTypeManager()->getStorage('taxonomy_term')->loadByProperties(['name' => $edit['name[0][value]']]);
+    $term = reset($terms);
+    $this->assertSession()->pageTextContains('Created new term ' . $term->getName() . '.');
+  }

loading the term by name to display it's name is a bit unecessary. imho it's sufficient to just check the success message based on $edit directly, we don't need to check that the term really exists?

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.

The last submitted patch, 29: 3277238-29.patch, failed testing. View results →

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

Seems Patch from #29 🐛 Fix \Drupal\taxonomy\Entity\Term::getName() to conform to the interface Needs work 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?

The last submitted patch, 29: 3277238-29.patch, failed testing. View results →

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.

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.

xjm → credited fjgarlin → .

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!

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