Expand TermInterface to provide methods

The properties are on https://api.drupal.org/api/drupal/core%21modules%21taxonomy%21lib%21Drup..., not the interface

Tagging as Novice, There are great instructions in the meta issue and there are a few references examples there that were already committed or have patches.

Think this deals with it.
"For NG entities, for example Node,
all of the public properties are deleted, and the init() method is deleted."

- removed public properties and init in Term
- added getter method signatures to TermInterface
- implemented in Term
- updated Term::id to use new getter method

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermInterface.phpundefined
@@ -14,4 +14,76 @@
+  public function getName();

Not sure about this, as it is the same as label() essentially, which we however don't use always, as it's alterable and everything.

Fine with keeping it for now :)

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermInterface.phpundefined
@@ -14,4 +14,76 @@
+  public function getTID();
...
+  public function getUUID();
...
+  public function getVID();

It is not necessary to duplicate these. We have the generic id(), bundle() and uuid() methods for these, I think that's fine.

Other than that, the getters looks good. Let's go a bit further:
- Add corresponding setters
- Let's do some example conversions to these methods to make sure that the methods work. I think term has fewer uses than other entity, so look at a few files in the taxonomy module and search for places that you can use methods now.

There is this standard (not everyone likes) which says you should camelcase.

getTID => getTid
getUUID => getUuid

If it didn't change...

Removed tid, uuid, vid getters as suggested at https://drupal.org/node/2016701#comment-7581619
Added setters
Replace instances of $term->whatever->value with $term->getWhatever()/$term->setWhatever() as appropriate in taxonomy.admin.inc, taxonomy.pages.inc and taxonomy.tokens.inc

Also, a comment on "Not sure about this, as it is the same as label() essentially, which we however don't use always, as it's alterable and everything." (https://drupal.org/node/2016701#comment-7581619) - my £0.02 is that using get*, set* is better than method names like label as they make explicit the intention of the caller of the code. Because label() is both a verb and a noun it is less clear what is meant when you're calling it (are you labelling something? or getting a label?)

Looks good, just some minor feedback.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -178,7 +172,7 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
-    if (isset($this->parent->value)) {
+    if ($this->getParent() !== NULL) {

Not sure if the explicit NULL check is necessary here, might even be wrong as the default value I think is 0, which means no parent. So setting to 0 is a valid operation and means, no parents.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermInterface.phpundefined
@@ -14,4 +14,104 @@
+   * Returns Term's description.
+   * ¶
+   * @access public

Some trailing spaces, make sure your IDE is configured to remove them, or look at it in dreditor to see them.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermInterface.phpundefined
@@ -14,4 +14,104 @@
+   * @access public

We don't use @access.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermInterface.phpundefined
@@ -14,4 +14,104 @@
+   * @return int|NULL
+   *   The parent term or NULL if parent not set
+   */
+  public function getParent();

What exactly does it return? I think the ID.

what happens with multiple parents?

So, with multiple parents $this->get('parent')->value just returns one value, so am thinking of removing getParent and creating
hasParents() - to indicate whether it has parents
getParents() - returns an array of parent term ids

You should be able to loop over $this->get('parent') to get multiple values, and count($this->get('parent')) should also work.

Thanks! Fixed (hopefully).

Can you provide an interdiff?

See https://drupal.org/documentation/git/interdiff

interdiff

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -178,7 +178,7 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
-        '#default_value' => $term->getParent(),
+        '#default_value' => $term->getParents(),

@@ -329,12 +329,12 @@ function taxonomy_overview_terms_submit($form, &$form_state) {
-    if ($term->getParent() == 0 && $term->getWeight() != $weight) {
+    if (count($term->getParents()) == 0 && $term->getWeight() != $weight) {
       $term->setWeight($weight);

This won't work.

I think we need both, maybe a getParent() or getFirstParent() that returns just the first parent?

Getting closer - added here for comments/review - marked as not-for-test.

- we now have getSoleParent which, if there is just one parent returns the parent's tid, otherwise FALSE
- clearParents removes all parents
- addParents adds one or more parents
- parents are loaded in the storagecontroller's attachload method. thinking about it as i write this, this should properly be defined on TermStorageControllerInterface as loadParents and called whenever, to abstract that away from any dependencies on the things the storage controller might or might not do
- modified some of the admin pages to take account of this. Currently too much of the loading of parents, calculating depths etc is done here and not as part of an API on term or vocabulary - will continue to work on this.

This will need a re-roll now

rerolled
-------------------
June 28, 2013
f36e523241dc3d35a7ff244d51fa59addc817698

Note:

deleted by us: core/modules/taxonomy/taxonomy.admin.inc was seen in the output of git status during the re-roll as a result core/modules/taxonomy/taxonomy.admin.inc is not present in this patch.

Going to look at this now.

I don't like the $parent variable on Term - it's doing too many things. Currently, the description of this variable is:

  /**
   * The parent term(s) for this term.
   *
   * This property is not loaded, but may be used to modify the term parents via
   * Term::save().
   *
   * The property can be set to an array of term IDs. An entry of 0 means this
   * term does not have any parents. When omitting this variable during an
   * update, the existing hierarchy for the term remains unchanged.
   */

The upshot of this now Term is an object is that the getParents() method returns either an int 0 (meaning no parents and we need an update) or an array of Term tids which represent the new parents of the term or NULL meaning the parents have not been modified (there may or may not be any parents). I think getParents() should always return an array of the parent Term objects and the empty array in the case of there being no parents. Keeping track of whether the parent terms have changed is an internal function of the Term object and should be hidden.

Does this sound OK to try and fix as part of this issue?

I'm at the DrupalCon Prague sprint day in Prague today if anyone wants to talk about this - ceng_ on irc.

OK, so $parent on the Term class can either be:

1. an array of the parent tids and we want to update next time Term is saved.
2. an array containing a single 0 value, meaning no parent tids but we still want to update on save. The 0 term tid needs to be passed to the storage controller during update as updateTermHierarchy uses it to insert a row into taxonomy_term_hierarchy to indicate there is no parents. Would be really good if this was not needed.
3. unset, meaning it may or may not have parent tids but we don't want to update them when the term is saved.

The attached patch rerolls #24 but starts to take into account the tri-state nature of the parent variable and so will (hopefully) pass the tests now.

I think that it would be good to do the following things:

1. Split out the responsibilities of the $parent variable so that the Term object keeps track of if the parents have been updated separately from the list of its current parents. I have started this process by adding a protected function called $term->hasChangedParents() which will return TRUE if they have changed.

2. Agree that the TermInterface getParents() method should return an array of Term objects rather than an array of parent tids only if they have changed and not include the special 0 tid meaning no parents. If there are no parents it should return the empty array.

3. Ideally stop putting entries into taxonomy_term_hierarchy table for terms with no parents. This is likely to have wider implications - doing this right now means terms arn't loaded, probably because of a db select statement which joins on this table somewhere.

At DrupalCon Prague and available for discussion - ceng_ on irc

switching to needs review

Ah, where would we be without tests?!

Attached patch fixes a bug noticed by the test code (missing setting of a variable) and updates all tests which directly access Term object parameters to use methods instead (that I could find).

Please note the questions and approach in #29 needs discussion.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.php
@@ -17,6 +17,20 @@
+    foreach ($terms as $idx => $term) {
+      $parents = array_keys(taxonomy_term_load_parents($term->id()));
+      $term->setParents($parents);
+    }

We explicitly avoid this at the moment for performance reasons. We load and save possibly many terms (e.g. when re-ordering them). We don't want to load all terms.

I'd really like to avoid making functional changes here. Can we possibly ignore the parent stuff completely here?

I've removed all the changes to the parent variable and removed all the *Parents() methods.

I can't interdiff this patch for some reason, one of it's hunks has failed - I guess the patches are too different now?

Tests are passing locally so hopefully this will go green.

@berdir, thanks for the review - shall I go create a new issue for the parents variable to clean up the code?

Final question, am I right in thinking we need to remove the public variables on the class and the init() function which unsets them (rather than set their modifiers to private/protected?)

Setting to needs review

Removing the public properties and init() is the correct thing to do for content (NG) entities. We actually don't want them , we need them to go through the magic __get()/__set() (hence the init()). Those properties are a cheat to make auto-complete in IDE's work when type hinted on the *class*. But we added interfaces and type hint on interfaces now, which makes them useless. They're quite confusing, so we're removing them and replacing them with methods.

1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
   @@ -57,91 +57,92 @@
      /**
   -   * The taxonomy term ID.
   +   * The parent term(s) for this term.
   +   *
   +   * This property is not loaded, but may be used to modify the term parents via
   +   * Term::save().
   +   *
   +   * The property can be set to an array of term IDs. An entry of 0 means this
   +   * term does not have any parents. When omitting this variable during an
   +   * update, the existing hierarchy for the term remains unchanged.
       *
       * @var \Drupal\Core\Entity\Field\FieldItemListInterface
       */
   -  public $tid;
   +  public $parent;
   

   Let's remove the parent too. It's nice documentation to have, but we need this property to be gone for it to work. Very interesting that this doesn't fail, tells me again that our test coverage of that stuff is non-existing :)

2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
   @@ -57,91 +57,92 @@
   -   * The taxonomy vocabulary ID this term belongs to.
   -   *
   -   * @var \Drupal\Core\Entity\Field\FieldItemListInterface
   +   * Overides \Drupal\Core\Entity\EntityNG::init().
       */
   -  public $vid;
   +  protected function init() {
   +    parent::init();
   +    unset($this->parent);
   +  }
    
   

   And this then too. I think it's ok to not have a method for it for the moment.

3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermInterface.php
   @@ -15,4 +15,79 @@
   +   * Returns Term's description.
   

   Term should be lowercase I think, as the others are.

4. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermInterface.php
   @@ -15,4 +15,79 @@
   +   * @return \Drupal\taxonomy\Entity\Term
   

   Returns should be TermInterface instead of Entity\Term.

5. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/RssTest.php
   @@ -100,7 +100,7 @@ function testTaxonomyRss() {
   diff --git a/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php
   
   diff --git a/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php
   index 7722a16..00ec21e 100644
   
   index 7722a16..00ec21e 100644
   --- a/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php
   
   --- a/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php
   @@ -57,4 +57,3 @@ function testTermIndentation() {
   
   @@ -57,4 +57,3 @@ function testTermIndentation() {
      }
    
    }
   -
   

   Unrelated change, no other changes in that file so let's not include that.

6. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TokenReplaceTest.php
   @@ -119,9 +119,9 @@ function testTaxonomyTokenReplacement() {
        // Generate and test unsanitized tokens.
   ...
   +    $tests['[term:parent:name]'] = $term1->label();
   

   This very explicitly says name, so let's use getName() for those.

Thanks for the review @berdir

1. Removed $parent property
2. Removed init method
3. Fixed case in the TermInterface comments
4. Comments for setters @return TermInterface now
5. Removed coding standards fixes in TaxonomyTermIndentationTest.php and VocabularyUnitTest.php as there was no other changes
6. Replaced ->label() function calls with ->getName()

Setting to needs review (I will learn to do this at the same time as posting the patch one day...)

Looks great, so close :)

1. +++ a/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.php
   @@ -57,3 +57,4 @@
   +
   

   Now there's a new line in there.

   Would have marked it RTBC anyway, but...

2. +++ a/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyUnitTest.php
   @@ -191,7 +191,7 @@
          'type' => 'text',
   +      'cardinality' => 4
   -      'cardinality' => 4,
        );
   

   This change is wrong and unnecessary.

Hey @berdir - thanks for the comments but I am pretty sure you are looking at the interdiff which is showing these things being reset from the previous patch! The patch #38 does not contain those two items.

Hope this is ok now!

Ouch. You are of course right, sorry about that! Was a bit in a hurry and tired.

Great, RTBC then! Might need a re-roll after #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface is in, though, as that changes the EntityNG class name.

Follow up issue to deal with the $parent property of the Term Entity class: #2100695: Taxonomy - Add accessor methods for term parents

OK, I'm new to this so I might be doing something wrong. But, it seems to apply cleanly to me. Don't think it needs a reroll - but, I could be wrong.

Yes, this doesn't need a re-roll right now, but will once the mentioned issue has been committed.

Rerolled. Can't produce an interdiff anymore.

1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
   @@ -57,91 +57,70 @@
   +   * Implements Drupal\Core\Entity\EntityInterface::id().
   

   @inheritdoc

2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TokenReplaceTest.php
   @@ -86,8 +86,8 @@ function testTaxonomyTokenReplacement() {
   +    $tests['[term:name]'] = check_plain($term1->getName());
   +    $tests['[term:description]'] = check_markup($term1->getDescription(), $term1->getFormat());
   
   @@ -101,11 +101,11 @@ function testTaxonomyTokenReplacement() {
   +    $tests['[term:name]'] = check_plain($term2->getName());
   +    $tests['[term:description]'] = check_markup($term2->getDescription(), $term2->getFormat());
   ...
   +    $tests['[term:parent:name]'] = check_plain($term1->getName());
   
   +++ b/core/modules/taxonomy/taxonomy.tokens.inc
   @@ -104,11 +104,11 @@ function taxonomy_tokens($type, $tokens, array $data = array(), array $options =
   +          $replacements[$original] = $sanitize ? check_plain($term->getName()) : $term->getName();
   ...
   +          $replacements[$original] = $sanitize ? check_markup($term->getDescription(), $term->getFormat(), '', TRUE) : $term->getDescription();
   
   @@ -132,7 +132,7 @@ function taxonomy_tokens($type, $tokens, array $data = array(), array $options =
   +            $replacements[$original] = check_plain($parent->getName());
   

   We can use String:: functions here.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.php
@@ -76,7 +76,7 @@ public function testEntityReferenceItem() {
+    $this->assertEqual($entity->field_test_taxonomy->entity->name->value, $this->term->getName());

@@ -98,7 +98,7 @@ public function testEntityReferenceItem() {
+    $this->assertEqual($entity->field_test_taxonomy->entity->name->value, $term2->getName());

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermReferenceItemTest.php
@@ -108,7 +108,7 @@ public function testTaxonomyTermReferenceItem() {
+    $this->assertEqual($entity->field_test_taxonomy->entity->name->value, $term2->getName());

I think ->name->value<code> can be converted to <code>->getName() too.

Also grepping the code for for unconverted things shows me that the result of the conversion is not exactly consistent - take for example testTaxonomyTermReferenceItem()

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermReferenceItemTest.php
@@ -96,7 +96,7 @@ public function testTaxonomyTermReferenceItem() {
     $entity->field_test_taxonomy->entity->save();
     // Verify it is the correct name.
     $term = entity_load('taxonomy_term', $tid);
-    $this->assertEqual($term->name->value, $new_name);
+    $this->assertEqual($term->getName(), $new_name);
 
     // Make sure the computed term reflects updates to the term id.
     $term2 = entity_create('taxonomy_term', array(
@@ -108,7 +108,7 @@ public function testTaxonomyTermReferenceItem() {

@@ -108,7 +108,7 @@ public function testTaxonomyTermReferenceItem() {
 
     $entity->field_test_taxonomy->target_id = $term2->id();
     $this->assertEqual($entity->field_test_taxonomy->entity->id(), $term2->id());
-    $this->assertEqual($entity->field_test_taxonomy->entity->name->value, $term2->name->value);
+    $this->assertEqual($entity->field_test_taxonomy->entity->name->value, $term2->getName());
   }

We've converted these but earlier in the code we do this
$this->assertEqual($entity->field_test_taxonomy->entity->name->value, $this->term->name->value); ... both sides of this assertion can be changed to use the new getName() method. And this $entity->field_test_taxonomy->entity->name = $new_name; should be using the setName() method.

In OverviewTerms::submitForm() we do $term->weight->value = $weight; - shouldn't this be using the setter? And there are places in this method where we should be using the getter too.

In taxonomy_term_feed() we do $channel['description'] = check_markup($term->description->value, $term->format->value, '', TRUE);

In testTermMultipleParentsInterface() we do $this->assertEqual($edit['description[value]'], $term->description->value, 'Term description was successfully saved.');

Thanks for the review @alexpott,

I've carefully grepp'd through the codebase looking for other places to use the setters and getter.

There are a number of places a $term variable is used but they are not term entities but results of a db_query. In these cases I renamed the variable from $term to $term_record to make it clear.

There was not getter or setter for the vocabulary vid so I have added these - getVocabularyId() and setVocabularyId($vocabulary_id)

Fixed the failing test and rerolled.

This test is passing locally ...

Here is a rerolled patch - maybe a glitch in the space-time continuum?

Missed jibran suggestions from #47

Updated a doc comment

Added the String::checkPlain instead of check_plain

All seems to look good. Few comments on it - mainly queries:

1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.php
   @@ -387,8 +387,8 @@ public function submitForm(array &$form, array &$form_state) {
   +      if ($term->parent->value == 0 && $term->getWeight() != $weight) {
   

   This is probably outside the scope of this task, but should $term->parent be accessed via a getter method?

2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/entity_reference/selection/TermSelection.php
   @@ -67,7 +68,7 @@ public function getReferenceableEntities($match = NULL, $match_operator = 'CONTA
   +            $options[$vocabulary->id()][$term->id()] = str_repeat('-', $term->depth) . String::checkPlain($term->getName());
   

   Shouldn't $term->depth be accessed via a getter method?

3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermInterface.php
   @@ -15,4 +15,99 @@
   +   * @return \Drupal\taxonomy\TermInterface
   

   Based on some comments that I have had before. If a method returns the object it was called on, we document this with @return self. This needs to be changed multiple times.

parent is weird, we decided to ignore that here, there's a follow-up issue that will try to deal with it.

depth as well, it's not an actual field, it's just something tack onto terms in taxonomy_get_tree() or however that function was called.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.php
@@ -172,13 +173,7 @@ public function buildEntity(array $form, array &$form_state) {
 
...
     // Prevent leading and trailing spaces in term names.
-    $term->name->value = trim($term->name->value);
-
-    // Convert text_format field into values expected by
-    // \Drupal\Core\Entity\Entity::save() method.
-    $description = $form_state['values']['description'];
-    $term->description->value = $description['value'];
-    $term->description->format = $description['format'];
+    $term->setName(trim($term->getName()));

While this is a bit out of context of this, I had a conflict here and we could have deleted this already when converting description to a text field, because it actually does match the expected values now. win :)

57: drupal8.taxonomy-module.2016701-57.patch queued for re-testing.

I did a review for this issue and I think it looks very good.
I have fixed the patch (I hope) and I have a couple of remarks/questions.

1. You forgot the update for check_plain a couple times (they are in my patch)

core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TokenReplaceTest.php line 87
core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTest.php lines 265, 266, 274 and 290

2. You are probable right, but I am not sure, about the construction:

$entity->field_test_taxonomy_term->entity->getName()

In the file: core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.php

line 93
$this->assertEqual($entity->field_test_taxonomy_term->entity->getName(), $this->term->getName());

line 115
$this->assertEqual($entity->field_test_taxonomy_term->entity->getName(), $term2->getName());

line 99
$entity->field_test_taxonomy_term->entity->setName($new_name);

3. In the file: core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
is the variable "public $parent" is gone, but there are no functions for interactions

4. In the file: core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.php

- // Convert text_format field into values expected by
- // \Drupal\Core\Entity\Entity::save() method.
- $description = $form_state['values']['description'];
- $term->description->value = $description['value'];
- $term->description->format = $description['format'];

You are removing these lines. Why?

Thanks for the re-roll.

Make sure to always include a interdiff when you make changes, so that it is possible to see what you changed. Also make sure to change an issue to needs review so that it can be tested.

2. That's correct.
3. That is fine, because parent is weird. There's a follow-up issue for it
4. See #58.

Re-roll and fixed the double-use of the same class.

Another re-roll and found two more instances to update.

Updating @return's for new coding standard and remove setVocabularyId(), this is a read-only field, you can't change the vocabulary of a term.

Primary questions:
1) why use getName() - there's label()
2) getVocabularyId() vs bundle()

Also found some out of scope clean-ups check_plain() to String::checkPlain()
and a set of nitpicks...

1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
   @@ -254,7 +173,74 @@ public static function baseFieldDefinitions($entity_type) {
   +  public function getDescription() {
   +    return $this->get('description')->value;
   ...
   +  public function setDescription($description) {
   +    $this->set('description', $description);
   ...
   +  public function getFormat() {
   +    return $this->get('description')->format;
   ...
   +  public function setFormat($format) {
   +    $this->get('description')->format = $format;
   

   Is this needed after #569434: Remove taxonomy term description field; provide description field for forum taxonomy

2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.php
   @@ -427,9 +427,9 @@ public function submitForm(array &$form, array &$form_state) {
   -      if ($term->parents[0] == 0 && $term->weight->value != $weight) {
   -        $term->parent->value = $term->parents[0];
   ...
   +      if ($term->parents[0] == 0 && $term->getWeight() != $weight) {
   +        $term->parent->value = $term->parent->value;
   

   second line looks weird

3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldFormatter/PlainFormatter.php
   @@ -8,6 +8,7 @@
   +use Drupal\Component\Utility\String;
   
   @@ -30,7 +31,7 @@ public function viewElements(FieldItemListInterface $items) {
   -        '#markup' => check_plain($item->entity->label()),
   +        '#markup' => String::checkPlain($item->entity->label()),
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument/IndexTid.php
   @@ -8,6 +8,7 @@
   +use Drupal\Component\Utility\String;
   
   @@ -24,8 +25,8 @@ public function titleQuery() {
   -      $titles[] = check_plain($term->name);
   ...
   +      $titles[] = String::checkPlain($term_record->name);
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument/IndexTidDepth.php
   @@ -8,6 +8,7 @@
   +use Drupal\Component\Utility\String;
   
   @@ -121,7 +122,7 @@ public function query($group_by = FALSE) {
   -      return check_plain($term->label());
   +      return String::checkPlain($term->label());
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument/Taxonomy.php
   @@ -8,6 +8,7 @@
   +use Drupal\Component\Utility\String;
   
   @@ -26,7 +27,7 @@ function title() {
   -        return check_plain($term->label());
   +        return String::checkPlain($term->label());
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument/VocabularyVid.php
   @@ -8,6 +8,7 @@
   +use Drupal\Component\Utility\String;
   
   @@ -24,7 +25,7 @@ class VocabularyVid extends Numeric {
   -      return check_plain($vocabulary->label());
   +      return String::checkPlain($vocabulary->label());
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/field/TaxonomyIndexTid.php
   @@ -115,15 +116,15 @@ public function preRender(&$values) {
   -      foreach ($result as $term) {
   ...
   +      foreach ($result as $term_record) {
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -111,11 +111,11 @@ protected function valueForm(&$form, &$form_state) {
   -        foreach ($result as $term) {
   +        foreach ($result as $term_record) {
   
   @@ -136,9 +136,9 @@ protected function valueForm(&$form, &$form_state) {
   -          foreach ($tree as $term) {
   +          foreach ($tree as $term_record) {
   
   @@ -155,8 +155,8 @@ protected function valueForm(&$form, &$form_state) {
   -        foreach ($result as $term) {
   ...
   +        foreach ($result as $term_record) {
   
   @@ -316,9 +316,9 @@ function validate_term_strings(&$form, $values) {
   -    foreach ($result as $term) {
   ...
   +    foreach ($result as $term_record) {
   
   @@ -357,8 +357,8 @@ public function adminSummary() {
   -      foreach ($result as $term) {
   ...
   +      foreach ($result as $term_record) {
   

   out of scope, but makes sense

4. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.php
   @@ -172,13 +173,7 @@ public function buildEntity(array $form, array &$form_state) {
   -    // Convert text_format field into values expected by
   -    // \Drupal\Core\Entity\Entity::save() method.
   -    $description = $form_state['values']['description'];
   -    $term->description->value = $description['value'];
   -    $term->description->format = $description['format'];
   

   there's no conversion? just removed?

5. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermReferenceItemTest.php
   @@ -87,29 +87,29 @@ public function testTaxonomyTermReferenceItem() {
   -    $this->assertEqual($entity->field_test_taxonomy->entity->name->value, $this->term->name->value, 'Field item entity contains the expected name.');
   +    $this->assertEqual($entity->field_test_taxonomy->entity->name->value, $this->term->getName(), 'Field item entity contains the expected name.');
   ...
   -    $entity->field_test_taxonomy->entity->name = $new_name;
   +    $entity->field_test_taxonomy->entity->setName($new_name);
   

   $entity->field_test_taxonomy->entity->name->value maybe needs conversion too as second hunk

6. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermReferenceItemTest.php
   @@ -87,29 +87,29 @@ public function testTaxonomyTermReferenceItem() {
   -      'vid' => $this->term->bundle(),
   +      'vid' => $this->term->getVocabularyId(),
   

   any reason to replace short bundle() with getVocabularyId()?

7. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TokenReplaceTest.php
   @@ -60,10 +61,6 @@ function setUp() {
   -  /**
   -   * Creates some terms and a node, then tests the tokens generated from them.
   -   */
      function testTaxonomyTokenReplacement() {
   

   no reason to remove doc block

8. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.php
   @@ -80,7 +80,7 @@ function testVocabularyPermissionsTaxonomyTerm() {
   -    $this->assertText($term->name->value, 'Edit taxonomy term form opened successfully.');
   +    $this->assertText($term->getName(), 'Edit taxonomy term form opened successfully.');
   
   @@ -107,11 +107,11 @@ function testVocabularyPermissionsTaxonomyTerm() {
   -    $this->assertRaw(t('Are you sure you want to delete the term %name?', array('%name' => $term->label())), 'Delete taxonomy term form opened successfully.');
   +    $this->assertRaw(t('Are you sure you want to delete the term %name?', array('%name' => $term->getName())), 'Delete taxonomy term form opened successfully.');
   ...
   -    $this->assertRaw(t('Deleted term %name.', array('%name' => $term->label())), 'Term deleted.');
   +    $this->assertRaw(t('Deleted term %name.', array('%name' => $term->getName())), 'Term deleted.');
   

   getName() vs standard label()

9. +++ b/core/modules/taxonomy/taxonomy.views.inc
   @@ -7,6 +7,7 @@
   +use Drupal\Component\Utility\String;
   

   what for?

Re-roll.

1)/2) As discussed. This is a common pattern now, we've done it for nodes and comments too. Those methods provide more context when you are working with a $term as opposed to an $entity that can be a term or something else. Code like 'term-vocabulary-' . $term->bundle() is much less self-explaining than 'term-vocabulary-' . $term->getVocabularyId().

1. No, it will no longer be necessary then but that issue didn't happen yet. We could not add methods, but it is not guaranteed that the other issue will happen.

2. Agreed, reverted. Test coverage for all that crazy overview/re-order code is not sufficient...

3. Yeah, the check plains wouldn't have been necessary, the $term_record rename is useful in this context because that code makes it hard to find occurrences and identify places where $term isn't a Term object. It should always be, but that's not something we can fix here.

4. Yes because that code is not necessary, due to changing description to a text field, the correct value and format is already automatically assigned.

5. Changed.

6. Because it makes more sense as explained above. We explicitly want the vocabulary id, not the abstract bundle thing. I could also leave it out, the reason it's there is another issue changed it to bundle() and I kept the change when I re-rolled.

7. Yeah, no idea where that came from. Reverted. Also fixed a bunch of wrong file permission changes.

8. Same as above. Agreed that those examples are a bit special, for example because the name can't be overridden anymore.

9. Removed.

Looks all addressed, also discussed at IRC that $term vs $term_record makes sense for further clean-up.
Suppose we need one more novice follow-up to consistently use getName() instead of label()

Sorry, #2145103: Provide non-configurable field types for entity created, changed and timestamp fields just broke this, so it needs another re-roll.

I read through the patch though and it looks fine, although I'm still confused on why we retain a few ->label() hunks despite having the getName() method.

Re-rolled.

Well, I left them out to keep the patch smaller, here's one with label() renamed to getName() everywhere I found it. As you can see, it doubles the patch size ;)

Search & replace fail.

It take a time review a mess of interdiffs, as there's no actual changes back to rtbc

Committed a8a8b2c and pushed to 8.x. Thanks!

We need to search existing change notices and check that are up to date after this change.