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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett 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
CreditAttribution: andypost commented
Category: task » bug
Priority: Normal » Major
Status: Active » Needs review
FileSize
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
CreditAttribution: tim.plunkett 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
CreditAttribution: andypost commented
FileSize
1848904-core-et-test-bundle-4.patch4.5 KB

Here's a re-roll, fixed #3

YesCT’s picture

YesCT CreditAttribution: 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
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt1.41 KB
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 CreditAttribution: YesCT commented
Status: Needs review » Reviewed & tested by the community
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett 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
CreditAttribution: tim.plunkett 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
CreditAttribution: andypost 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
CreditAttribution: andypost 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
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
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
CreditAttribution: andypost commented
Status: Needs work » Needs review

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

YesCT’s picture

YesCT CreditAttribution: 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 🇨🇦
CreditAttribution: webchick 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) CreditAttribution: commented
Issue summary: View changes

Updated summary