Working on #1552396-140: Convert vocabularies into configuration
I need to pas a different bundle name to create a term but it's always related to $this->bundle.

The test should to be improved to be usable for entities with bundles.

Patch changes base class EntityTranslationUITest method createEntity() by adding 3rd argument as it used in CommentTranslationUITest::createEntity()

CommentFileSizeAuthor
#13 1848904-core-et-test-bundle-13.patch4.81 KBandypost
#6 drupal-1848904-5.patch4.63 KBtim.plunkett
#6 interdiff.txt1.41 KBtim.plunkett
#4 1848904-core-et-test-bundle-4.patch4.5 KBandypost
#2 1848904-core-et-test-bundle-1.patch3.87 KBandypost

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Category: bug » task
Priority: Major » Normal

That's something that can be worked around, right? I agree it'd be nice to clean up though.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Category: task » bug
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new 1848904-core-et-test-bundle-1.patch3.87 KB

Here's a fixed tests

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTranslationUITest.phpundefined
@@ -51,7 +51,7 @@ protected function setupBundle() {
+    $this->vocabulary = entity_create('taxonomy_vocabulary', array(

This class now needs a protected $vocabulary; with docblock.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.phpundefined
@@ -231,16 +231,18 @@ function testTranslationUI() {
+   *   (optional) Entity bundle if entity has bundles. Default to $this->bundle.

Defaults to NULL.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.phpundefined
@@ -231,16 +231,18 @@ function testTranslationUI() {
+      $entity_values[$info['entity_keys']['bundle']] = isset($bundle_name) ? $bundle_name : $this->bundle;

This could just be = $bundle_name ?: $this->bundle;

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1848904-core-et-test-bundle-4.patch4.5 KB

Here's a re-roll, fixed #3

yesct’s picture

yesct commented

how about

+   *   (optional) Entity bundle if entity has bundles. Defaults to NULL. 
+    *    Set to $this->bundle if no argument given.

Or something like that.

Otherwise, coding standards and docs wise this looks good. And the changes make sense.

Is #3

This class now needs a protected $vocabulary; with docblock.

a followup but not directly needed to fix these tests?

I did not manually try the tests.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new interdiff.txt1.41 KB
new drupal-1848904-5.patch4.63 KB

Missed the @var part. YesCT mentioned that it might be helpful to explain what leaving $bundle_name NULL would do.

yesct’s picture

yesct commented
Status: Needs review » Reviewed & tested by the community
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Category: bug » task

This is not a bug because something is broken, it's just blocking another major because it is inflexible.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Bundle always overriden by this->bundle in tests » Bundles cannot be specified in Entity Translation tests

Whoops, missed the rename as well.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Bundles cannot be specified in Entity Translation tests » Bundle always overriden by this->bundle in tests
Priority: Major » Normal

Yes, this normal

also CommentTranslationUITest::createEntity($values, $langcode, $node_bundle = NULL) already uses 3rd arbument

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Bundle always overriden by this->bundle in tests » Bundles cannot be specified in Entity Translation tests

x-post

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-1848904-5.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 1848904-core-et-test-bundle-13.patch4.81 KB

Got it, term required bith vid and vocabulary_machine_name to be saved, so override method should be here

Status: Needs review » Needs work

The last submitted patch, 1848904-core-et-test-bundle-13.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

#13: 1848904-core-et-test-bundle-13.patch queued for re-testing.

yesct’s picture

yesct commented
Status: Needs review » Reviewed & tested by the community

I checked the code again. The changes were made and it's still looking very good.

The issue summary needs to be clarified to address tim and andy's conversation regarding the approach.
It's not required, but using the issue summary template may result in making it easier on a committer when they come to evaluate this RTBC.
http://drupal.org/node/1155816

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated summary