Expand Vocabulary with methods

Initial patch. This doesn't really work - the interface is in place but now vocabularies save but we don't have full objects with names and descriptions etc. Need to dig some more to work out what earth is going on. Note I also had to added some new properties to other interfaces to get tests even to run not sure if we may want to split those elements out?

Ok no local fatals on taxonomy test - still some tests fails and exceptions to fix but hopefully the bot can do a full test. I'll try and fix up the other stuff tomorrow.

Well that was easier than excepted all taxonomy test are now green locally.

New version which hopefully fixes the failures

#7: vocab-interface-7.patch queued for re-testing.

This needs work I just haven't got round to looking at it again - there are a couple of fatals that still need solving.

@marcingy is it ok to unassign this until you are ready to restart work on it? Did you have any questions?

@marcingy: I think that all the fatals that you had in the summer are now gone. Are you still willing to work on this issue?

7: vocab-interface-7.patch queued for re-testing.

Recreated patch.

16: vocab-interface-16.patch queued for re-testing.

This patch resolved the issue of why testbot fails to install ... nothing more, patches here have never come back green.

I have removed the setUuid() requirement inserted into the the ConfigEntityInterface definition
it is well outside the scope of a taxonomy submodule patch

Specifically the reason it break installation is that it has knock on implications for many entities that extends ConfigEntityBase

The one that install breaks on first is -

class DateFormat extends ConfigEntityBase implements DateFormatInterface {

Moving to need review just to trigger testbot

tried to apply this patch and didn't work

Reroll.

Previous patch worked but was following PSR-0. Rerolled for PSR-4 as per instructions here:

https://groups.drupal.org/node/424758

Looks good to me.

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -95,6 +95,71 @@ public function id() {
+  /**
+   * {@inheritdoc}
+   */
+  public function setNewVid($machineName) {
+    return $this->set('vid', $machineName);
+  }
+

Do we need the "New" here? I think "setVid" would be ok. Thoughts?

There's no setVid so I think it's safe to assume we don't want a setNewVid.

Please review attached patch for fixes in #27.

I think the proper test here is mark all vocabulary properties as protected

1. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
   @@ -95,6 +95,71 @@ public function id() {
   +  public function setUuid() {
   

   is this really needed?

2. +++ b/core/modules/taxonomy/src/Tests/EfqTest.php
   @@ -61,7 +61,7 @@ function testTaxonomyEfq() {
   -    $this->assertEqual(array_keys($terms2), $result, format_string('Taxonomy terms from the %name vocabulary were retrieved by entity query.', array('%name' => $vocabulary2->name)));
   +    $this->assertEqual(array_keys($terms2), $result, format_string('Taxonomy terms from the %name vocabulary were retrieved by entity query.', array('%name' => $vocabulary2->label())));
   
   +++ b/core/modules/taxonomy/src/Tests/TokenReplaceTest.php
   @@ -93,7 +93,7 @@ function testTaxonomyTokenReplacement() {
   +    $tests['[term:vocabulary:name]'] = String::checkPlain($this->vocabulary->label());
   
   @@ -110,7 +110,7 @@ function testTaxonomyTokenReplacement() {
   +    $tests['[term:vocabulary:name]'] = String::checkPlain($this->vocabulary->label());
   
   @@ -124,7 +124,7 @@ function testTaxonomyTokenReplacement() {
   +    $tests['[term:vocabulary:name]'] = $this->vocabulary->label();
   
   @@ -134,8 +134,8 @@ function testTaxonomyTokenReplacement() {
   +    $tests['[vocabulary:name]'] = String::checkPlain($this->vocabulary->label());
   
   @@ -148,8 +148,8 @@ function testTaxonomyTokenReplacement() {
   +    $tests['[vocabulary:name]'] = $this->vocabulary->label();
   

   should be getName() not label()

3. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -855,6 +855,15 @@ public function uuid() {
   +  public function setUuid() {
   

   out of scope

I fixed the failing of test, please review.

Oh. This one should be green.

@undertext congratulations. you fixed it.

I won't advance the status of this issue because I have worked on it..
but here is partial review....

I have had a slow and steady scan of vocab-interface-2030669-42.patch

All changes look appropriate, and are within scope of the issue ... no funny business just changes I would have made so +1 from me.

1. +++ b/core/modules/taxonomy/src/VocabularyInterface.php
   @@ -14,4 +14,103 @@
   +   *
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called vocabulary entity.
   ...
   +   *
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called vocabulary entity.
   +   */
   ...
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called vocabulary entity.
   +   */
   ...
   +   *
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called vocabulary entity.
   

   Let's use @return $this directly.

2. +++ b/core/modules/taxonomy/src/VocabularyInterface.php
   @@ -14,4 +14,103 @@
   +   * @param integer $weight
   ...
   +   * @return integer
   ...
   +   * @param integer $hierarchy
   ...
   +   * @return integer
   

   Afaik we use int instead of integer

3. +++ b/core/modules/taxonomy/src/VocabularyInterface.php
   @@ -14,4 +14,103 @@
   +   * @param string $machineName
   ...
   +  public function setVid($machineName);
   

   let's use $machine_name

4. +++ b/core/modules/views/src/Tests/ViewsTaxonomyAutocompleteTest.php
   @@ -67,7 +67,7 @@ public function setUp() {
   -    $base_autocomplete_path = 'taxonomy/autocomplete_vid/' . $this->vocabulary->vid;
   +    $base_autocomplete_path = 'taxonomy/autocomplete_vid/' . $this->vocabulary->getVid();
   

   Do you have an oppinion of $this->vocabulary->getVid() vs. $this->vocabulary->id() ?

yeah, I didn't wanted to set it to RTBC directly.

Here is updated patch.

1. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
   @@ -95,6 +95,76 @@ public function id() {
   +  public function getVid() {
   +    return $this->get('vid');
   ...
   +  public function getName() {
   +    return $this->get('name');
   

   Do we really need this? there's id() and label() already

2. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
   @@ -95,6 +95,76 @@ public function id() {
   +  public function setVid($machineName) {
   

   we should hide the "vid", this is actually setId()

3. +++ b/core/modules/taxonomy/src/VocabularyInterface.php
   @@ -14,4 +14,103 @@
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called vocabulary entity.
   ...
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called vocabulary entity.
   ...
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called vocabulary entity.
   ...
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called vocabulary entity.
   ...
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called vocabulary entity.
   

   @return $this as said in #46 (1,2)

4. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -4,7 +4,6 @@
   -
   
   @@ -22,17 +21,17 @@
   -const TAXONOMY_HIERARCHY_DISABLED = 0;
   +    const TAXONOMY_HIERARCHY_DISABLED = 0;
   ...
   -const TAXONOMY_HIERARCHY_SINGLE = 1;
   +    const TAXONOMY_HIERARCHY_SINGLE = 1;
   ...
   -const TAXONOMY_HIERARCHY_MULTIPLE = 2;
   +    const TAXONOMY_HIERARCHY_MULTIPLE = 2;
   
   @@ -782,9 +781,9 @@ function taxonomy_build_node_index($node) {
   -          ->key(array('nid' => $node->id(), 'tid' => $tid))
   -          ->fields(array('sticky' => $sticky, 'created' => $node->getCreatedTime()))
   -          ->execute();
   +            ->key(array('nid' => $node->id(), 'tid' => $tid))
   +            ->fields(array('sticky' => $sticky, 'created' => $node->getCreatedTime()))
   +            ->execute();
   

   please do not change that

I did not find setId() function, hence keeping setVid, for the sake of similarity also keeping getVid.

@Sharique please provide interdiff.txt, it's really hard to follow the changes you doing
See https://www.drupal.org/node/1488712

Here is updated patch with interdiff.

Ahh, forgot change status again :(

Recreated patch to be able to apply on updated code base.

removing phpstorm file from patch.

Fixing php syntax error.

Reroll of #61.

Fixed TaxonomyPermissions->permissions():

Changed remaining references to ::$name, corrected method documentation.

Woot! Drupal installs now in the testbot. Now to fix the failing tests.

Tests pass locally for me. Trying testbot again.

removed some spacing that was added by patch in 66.
and corrected the failures.

bit tired so hopefully didn't mess anything up..

+++ b/core/modules/taxonomy/taxonomy.module
@@ -82,18 +82,42 @@ function taxonomy_help($route_name, RouteMatchInterface $route_match) {
+ * Implements hook_permission().
+ */
+function taxonomy_permission() {
+  $permissions = array(
+    'administer taxonomy' => array(
+      'title' => t('Administer vocabularies and terms'),
+    ),
+  );
+  foreach (entity_load_multiple('taxonomy_vocabulary') as $vocabulary) {
+    $permissions += array(
+      'edit terms in ' . $vocabulary->id() => array(
+        'title' => t('Edit terms in %vocabulary', array('%vocabulary' => $vocabulary->getName())),
+      ),
+    );
+    $permissions += array(
+       'delete terms in ' . $vocabulary->id() => array(
+         'title' => t('Delete terms from %vocabulary', array('%vocabulary' => $vocabulary->getName())),
+      ),
+    );
+  }
+  return $permissions;
+}
+

Why do you want to implement hook_permissions in this patch?

+++ b/core/modules/taxonomy/src/VocabularyInterface.php
@@ -15,4 +15,95 @@
+   /**
+    * Sets the vocabulary description.
+    *
+    * @param string description
+    *   The vocabulary description.
+    *
+    * @return $this
+    */
+   public function setDescription($description);
+

The return value must be set as:

 * @return \Drupal\taxonomy\VocabularyInterface
 * The called Vocabulary entity.

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -183,4 +139,118 @@ class Vocabulary extends ConfigEntityBundleBase implements VocabularyInterface {
+  public function setName($name) {
+      return $this->set('name', $name);
+  }
+

Must be:

  public function setName($name) {
    $this->set('name', $name);
    return $this;
  }

Also for the other set value functions.

The patch is a almost a month old, so also a reroll.

Hi

The hook_permission is introduced in patch #63 so we need an answer by mile23 what is going on there

Oh, that's very strange. Somehow it got inserted in.

Here's just a straight re-roll of #72. Will address the concerns in #73 after the testbot has its say.

Pretty sure the testbot failures are unrelated:

Drupal\Tests\Component\DrupalComponentTest                     2 passes
PHP Notice:  Undefined property: Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest::$results in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 650
[etc]

Changes from #73:

Removed taxonomy_permission().

Changed @return $this to @return \Drupal\taxonomy\VocabularyInterface in a few places, plus fixed some indentation errors in that file.

set() has a fluent interface, so it's fine to just say return $this->set('something', 'value');. No change.

......

Oops. named an interdiff as a patch.

All the fails in #79 (for the patch, not the interdiff) are still related to ContainerAwareEventDispatcherTest, which I'm pretty sure are unrelated to this issue.

If I understand Mile23 correctly. This issue is to be postponed on issue #2376039: Undefined property ContainerAwareEventDispatcherTest::results in run-tests.sh.

You understand correctly, and thanks for the reference.

The container test issue is fixed, setting to needs review

alexpott reverted the patch for #2376039: Undefined property ContainerAwareEventDispatcherTest::results in run-tests.sh. So putting it back to postponed. Sorry Mile23.

Doh! :-)

Fixed again #2376039: Undefined property ContainerAwareEventDispatcherTest::results in run-tests.sh

In the old patch was the function drupal_ucfirst used. The function has been deprecated.

I have reviewed the patch from #79. I have made some changes to the patch.
- Removed some spaces (to much indentation).
- Minor change to getName() comment.
- In five places I have changed the setVariableName() so that it return $this.
- In the patch the returning value of \Drupal::entityManager()->getStorage('field_storage_config')->loadMultiple($field_ids) is renamed to $fields. It was named $field_storages and everywhere else in drupal it is also called $field_storages. So I changed it back.

Everything else is RTBC for me.

1. +++ b/core/modules/taxonomy/src/VocabularyInterface.php
   @@ -15,4 +15,100 @@
   +  /**
   +   * Sets the vocabulary weight.
   +   *
   +   * @param integer $weight
   +   *   The weight of vocabulary.
   +   *
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called Vocabulary entity.
   +   */
   +  public function setWeight($weight);
   ...
   +  /**
   +   * Returns the vocabulary weight.
   +   *
   +   * @return integer
   +   *   The vocabulary weight.
   +   */
   +  public function getWeight();
   ...
   +  /**
   +   * Sets the vocabulary description.
   +   *
   +   * @param string description
   +   *   The vocabulary description.
   +   *
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called Vocabulary entity.
   +   */
   +  public function setDescription($description);
   ...
   +  /**
   +   * Sets the vocabulary vid.
   +   *
   +   * @param string $machineName
   +   *   The vocabulary machine name.
   +   *
   +   * @return \Drupal\taxonomy\VocabularyInterface
   +   *   The called Vocabulary entity.
   +   */
   +  public function setVid($machineName);
   

   None of these have any usages outside of tests therefore we should not be adding them. Use ->set() and ->get() instead.

2. +++ b/core/modules/taxonomy/src/VocabularyInterface.php
   @@ -15,4 +15,100 @@
   +  /**
   +   * Returns the vocabulary vid.
   +   *
   +   * @return string
   +   *   The vocabulary machine name.
   +   */
   +  public function getVid();
   

   This is not needed - just use the existing id() method instead.

3. +++ b/core/modules/views/src/Tests/ViewsTaxonomyAutocompleteTest.php
   @@ -67,7 +67,7 @@ protected function setUp() {
   -    $base_autocomplete_path = 'taxonomy/autocomplete_vid/' . $this->vocabulary->vid;
   +    $base_autocomplete_path = 'taxonomy/autocomplete_vid/' . $this->vocabulary->getVid();
   

   use ->id()

4. Nice to see everything getting protected

Bravely done what alexpott suggested.

Second try. Was the tests forgotten.

@daffie your interdiffs are diffs of diffs. This are interesting but difficult to read :)

For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff | Microbranching workflow: http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -183,4 +139,85 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
+  /**
+   * {@inheritdoc}
+   */
+  public function id() {
+    return $this->vid;
+  }

This is unnecessary. The parent class's id method uses voabulary's annotation to get the correct property.

Removed the function id().

Sorry about the interdiffs. I had some problems with the command. So I thought lets use the diff command then. That one is working for me. Did not know there was a difference. The interdiff command is working now. And I see the difference between the two.
The link to drupalgardens.com gives me an access denied. I probably need to make an account.
But thanks anyway I learned something new. :)

#100 is incorrect, the method is necessary, it already exists and was just moved around (not sure if that is a good idea, as it makes the patch bigger and harder to review).

the id is only automatically detected for content entities at the moment, where we have an automated mechanism that is actually faster than doing it manually.

@alexpott: Are you certain that id() can be removed. The patch is failing to test and the only difference with the patch from comment #98 is the removal of the function id(). Or is there something wrong with the testbot?

  /**
   * {@inheritdoc}
   */
  public function id() {
    return $this->vid;
  }

@Bendir: I am not so fast with typing. So I did not read your comment until after I posted comment #106.

Please review the patch from comment #98.

Yep I was wrong. We need the id() but we shouldn't be moving it. It's a bit annoying that we've got all this info in the entity_keys annotation and then don't use it.

@alexpott: You have put the status to "needs work". Why?

Because we shouldn't be moving the id() method in the Vocabulary class - we can leave it where it was - less to review.

@alexpott: The function id() was and is in the Vocabulary class. So where do you want to put the function id() then? Or do you want to add the function id() to ConfigEntityBase class.

  public function id() {
    return $this->getEntityKey('id');
  }

@daffie: At the same place inside that file, so that it is not removed and added again in the patch. same for postSave(). It is very hard now to see if postSave() actually changed or not.

The patch file is now more clear. There are no changes to id() and postSave().

There is a change here that I don't think is right, we shuldn't make the protected properties public again just because we remove the get/set functions for them?

@filijonka: The whole idea is that the class variables are encapsulated. That means that you must use functions to get or set the variables. And to make sure that you do not access them directly we make them protected.

yes but in last patch you have made several properties public again which you shouldn't. see #95 number 4.

@filijonka: I have looked at my patch again. And I have 5 class variables and they are all protected (vid, name, description, hierarchy and weight).

@daffie I trust you and apologizing, something must be wrong in my browser cause when I make review an patch 113 some says public.. Then at least we are on same page about that.

@filijonka: Sorry to hear that you have problems with your browser. Here is a part of the patch:

--- a/core/modules/taxonomy/src/Entity/Vocabulary.php
+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -52,21 +52,21 @@ class Vocabulary extends ConfigEntityBundleBase implements VocabularyInterface {
*
* @var string
*/
- public $vid;
+ protected $vid;

/**
* Name of the vocabulary.
*
* @var string
*/
- public $name;
+ protected $name;

/**
* Description of the vocabulary.
*
* @var string
*/
- public $description;
+ protected $description;

/**
* The type of hierarchy allowed within the vocabulary.
@@ -78,14 +78,51 @@ class Vocabulary extends ConfigEntityBundleBase implements VocabularyInterface {
*
* @var integer
*/
- public $hierarchy = TAXONOMY_HIERARCHY_DISABLED;
+ protected $hierarchy = TAXONOMY_HIERARCHY_DISABLED;

/**
* The weight of this vocabulary in relation to other vocabularies.
*
* @var integer
*/
- public $weight = 0;
+ protected $weight = 0;

I think the interdiffs are backwards.

Please use the patch file for the review.

As i understand comment #95, if we remove the setX then I would say it's probably safe to remove the getX to. Hence the call for getX in tests should be changed to ->get().

I did a search for getDescription for instance and I couldn't find any outside tests BUT please do one yourself.

The same as patch #113. Only with the function getDescription() removed.

Changed a description from a taxonomy entity. :(

The same as patch #113. Only with the function getDescription() removed.

please always add a interdiff, makes it easier for reviewers.

@filijonka: The requested interdiff.

@daffie You should always make an interdiff between each patch. and a patch is preferable to be made from the last one as a startpoint.

1. +++ b/core/modules/taxonomy/src/VocabularyForm.php
   @@ -48,7 +48,7 @@
   +      '#default_value' => $vocabulary->get('description'),
   

   Since this is outside a test the call should be as it was $vocabulary->getDescription().

   IF a protected property is only accessed in a test and never anywhere else we don't need the get* function but using ->get('propertyname'). But this isn't the case with descriotion.

2. +++ b/core/modules/taxonomy/src/VocabularyInterface.php
   @@ -58,10 +58,2 @@
   -  /**
   -   * Returns the vocabulary description.
   -   *
   -   * @return string
   -   *   The vocabulary description.
   -   */
   -  public function getDescription();
   -
   

   so this needs to be added again

probably my post that confused about the get function. sorry about that so we can disregard latest patch and go by comment #113.

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -78,14 +78,51 @@ class Vocabulary extends ConfigEntityBundleBase implements VocabularyInterface {
+  public function getDescription() {
+    return $this->get('description');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getHierarchy() {
+    return $this->get('hierarchy');
+  }

I don't like this simply because it's wrong imho. this should just be return $this->propertyname. We don't need to call for another function for that right?

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -78,14 +78,51 @@ class Vocabulary extends ConfigEntityBundleBase implements VocabularyInterface {
+    $this->set('hierarchy', $hierarchy);

Same goes for the set functions, we don't need to call for another function; just $this->propertyname=parametername, in above case simpy $this->hierarchy = $hierarchy

Please review :)

+++ b/core/modules/taxonomy/src/VocabularyInterface.php
@@ -15,4 +15,53 @@
+   * @return \Drupal\taxonomy\VocabularyInterface
+   *   The called Vocabulary entity.
...
+   * @return \Drupal\taxonomy\VocabularyInterface
+   *   The called Vocabulary entity.

Better is "@return $this"

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -78,14 +78,44 @@ class Vocabulary extends ConfigEntityBundleBase implements VocabularyInterface {
+  /**
+   * {@inheritdoc}
+   */
+  public function setName($name) {
+    $this->set('name', $name);
+    return $this;
+  }

+++ b/core/modules/taxonomy/src/VocabularyInterface.php
@@ -15,4 +15,53 @@
+  /**
+   * Sets the vocabulary name.
+   *
+   * @param string $name
+   *   The vocabulary name.
+   *
+   * @return \Drupal\taxonomy\VocabularyInterface
+   *   The called Vocabulary entity.
+   */
+  public function setName($name);

The function setName is only used once. So can we replace it with set('name', $name).

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -78,14 +78,44 @@ class Vocabulary extends ConfigEntityBundleBase implements VocabularyInterface {
+  /**
+   * {@inheritdoc}
+   */
+  public function getName() {
+    return $this->get('name');
+  }

+++ b/core/modules/taxonomy/src/VocabularyInterface.php
@@ -15,4 +15,53 @@
+  /**
+   * Returns the vocabulary name.
+   *
+   * @return string
+   *   The vocabulary name.
+   */
+  public function getName();

We have a function for this and it is called label(), so can you remove getName(). Change all getName()'s to label().

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceItemTest.php
@@ -183,7 +183,7 @@ public function testConfigEntityReferenceItem() {
+    $this->assertEqual($entity->field_test_taxonomy_vocabulary->entity->get('name'), $vocabulary2->get('name'));

+++ b/core/modules/forum/src/Tests/ForumTest.php
@@ -340,16 +340,16 @@ function editForumVocabulary() {
+    $this->assertEqual($current_vocabulary->get('name'), $edit['name'], 'The name was updated');
...
+    $current_vocabulary->set('name', $original_vocabulary->get('name'));
...
+    $this->assertEqual($current_vocabulary->get('name'), $original_vocabulary->get('name'), 'The original vocabulary settings were restored');

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateTaxonomyVocabularyTest.php
@@ -46,16 +46,16 @@ public function testTaxonomyVocabulary() {
+      $this->assertEqual($vocabulary->get('name'), "vocabulary $j (i=$i)");
...
+    $this->assertEqual($vocabulary->get('name'), 'vocabulary name much longer than thirty two characters');

+++ b/core/modules/taxonomy/src/Tests/TokenReplaceTest.php
@@ -88,7 +88,7 @@ function testTaxonomyTokenReplacement() {
+    $tests['[term:vocabulary:name]'] = String::checkPlain($this->vocabulary->get('name'));

@@ -105,7 +105,7 @@ function testTaxonomyTokenReplacement() {
+    $tests['[term:vocabulary:name]'] = String::checkPlain($this->vocabulary->get('name'));

@@ -119,7 +119,7 @@ function testTaxonomyTokenReplacement() {
+    $tests['[term:vocabulary:name]'] = $this->vocabulary->get('name');

@@ -129,8 +129,8 @@ function testTaxonomyTokenReplacement() {
+    $tests['[vocabulary:name]'] = String::checkPlain($this->vocabulary->get('name'));

@@ -143,8 +143,8 @@ function testTaxonomyTokenReplacement() {
+    $tests['[vocabulary:name]'] = $this->vocabulary->get('name');

+++ b/core/modules/taxonomy/src/Tests/VocabularyCrudTest.php
@@ -70,17 +70,17 @@ function testTaxonomyVocabularyDeleteWithTerms() {
+    $this->assertEqual($this->vocabulary->get('name'), $original_vocabulary->get('name'), 'Vocabulary loaded successfully.');
...
+    $this->assertEqual($new_vocabulary->get('name'), $vocabulary->get('name'), 'The vocabulary was loaded.');

@@ -129,12 +129,12 @@ function testTaxonomyVocabularyLoadMultiple() {
+    $vocabulary = current($controller->loadByProperties(array('name' => $vocabulary1->get('name'))));
...
+      'name' => $vocabulary2->get('name'),

+++ b/core/modules/taxonomy/src/Tests/VocabularyUiTest.php
@@ -137,12 +137,12 @@ function testTaxonomyAdminDeletingVocabulary() {
+    $this->assertRaw(t('Are you sure you want to delete the vocabulary %name?', array('%name' => $vocabulary->get('name'))), '[confirm deletion] Asks for confirmation.');
...
+    $this->assertRaw(t('Deleted vocabulary %name.', array('%name' => $vocabulary->get('name'))), 'Vocabulary deleted.');

Why not use label()?

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -225,9 +225,8 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
-        '#url' => $term->urlInfo(),
-      );
...
+      ) + $term->urlInfo()->toRenderArray();

Maybe this is a code improvement, but not necessary for this issue.

+++ b/core/modules/taxonomy/src/TaxonomyPermissions.php
@@ -14,6 +14,8 @@
+ *
+ * @see taxonomy.permissions.yml

@@ -44,21 +46,22 @@ public static function create(ContainerInterface $container) {
-   * Returns an array of REST permissions.
+   * Get taxonomy permissions.
...
+   *   Permissions array.

Maybe this is a documentation improvement, but not necessary for this issue.

+++ b/core/modules/forum/src/Tests/ForumTest.php
@@ -340,16 +340,16 @@ function editForumVocabulary() {
+    $this->assertEqual($current_vocabulary->get('description'), $edit['description'], 'The description was updated');
...
+    $current_vocabulary->set('description', $original_vocabulary->get('description'));

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateTaxonomyVocabularyTest.php
@@ -46,16 +46,16 @@ public function testTaxonomyVocabulary() {
+      $this->assertEqual($vocabulary->get('description'), "description of vocabulary $j (i=$i)");
...
+    $this->assertEqual($vocabulary->get('description'), 'description of vocabulary name much longer than thirty two characters');

+++ b/core/modules/taxonomy/src/Tests/TokenReplaceTest.php
@@ -129,8 +129,8 @@ function testTaxonomyTokenReplacement() {
+    $tests['[vocabulary:description]'] = Xss::filter($this->vocabulary->get('description'));

@@ -143,8 +143,8 @@ function testTaxonomyTokenReplacement() {
+    $tests['[vocabulary:description]'] = $this->vocabulary->get('description');

There is a new function called getDescription(). Use it!

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateTaxonomyVocabularyTest.php
@@ -46,16 +46,16 @@ public function testTaxonomyVocabulary() {
+      $this->assertEqual($vocabulary->get('hierarchy'), $i);
...
+    $this->assertEqual($vocabulary->get('hierarchy'), 3);

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -72,7 +72,7 @@ function testTaxonomyTermHierarchy() {
+    $this->assertEqual(0, $vocabulary->get('hierarchy'), 'Vocabulary is flat.');

Why not use getHierarchy()?

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -78,14 +78,44 @@ class Vocabulary extends ConfigEntityBundleBase implements VocabularyInterface {
+    return $this->get('hierarchy');

Can be changed to: return $this->hierarchy;

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -78,14 +78,44 @@ class Vocabulary extends ConfigEntityBundleBase implements VocabularyInterface {
+    $this->set('hierarchy', $hierarchy);

Can be changed to: $this->hierarchy = $hierarchy;

wow! thank you :)

Almost there.

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -72,7 +72,7 @@ function testTaxonomyTermHierarchy() {
+    $this->assertEqual(0, $vocabulary->gethierarchy(), 'Vocabulary is flat.');

It is getHierarchy().

+++ b/core/modules/taxonomy/src/VocabularyInterface.php
@@ -15,4 +15,34 @@
+   *   The called Vocabulary entity.

You can remove this line.

+++ b/core/modules/system/src/Tests/Entity/EntityCrudHookTest.php
@@ -449,8 +449,8 @@ public function testTaxonomyVocabularyHooks() {
-    $GLOBALS['entity_crud_hook_test'] = array();
...
+    $_SESSION['entity_crud_hook_test'] = array();

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -225,9 +225,8 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
-        '#url' => $term->urlInfo(),
-      );
...
+      ) + $term->urlInfo()->toRenderArray();

These changes are not necessary for this issue. So please remove them. If you feel they are necessary then create a separate issue for them.

I think that alexpott will not commit the last modification, it better a separate issue.

see the last block in comment #140

I would also like to know why the comment to the @return should be taken away?

see the last block in comment #140

I think that alexpott will not commit the last block, it better a separate issue.

I think that we have a misunderstanding. What I want is that you remove a change from your patch. The situation in the file core/modules/taxonomy/src/Form/OverviewTerms.php is without the patch from this issue applied is:

    $form['terms'][$key]['term'] = array(
      '#prefix' => !empty($indentation) ? drupal_render($indentation) : '',
      '#type' => 'link',
      '#title' => $term->getName(),
      '#url' => $term->urlInfo(),
    );

With the patch from comment #141 applied it will become:

    $form['terms'][$key]['term'] = array(
      '#prefix' => !empty($indentation) ? drupal_render($indentation) : '',
      '#type' => 'link',
      '#title' => $term->getName(),
    ) + $term->urlInfo()->toRenderArray();

The same for change in the file: core/modules/system/src/Tests/Entity/EntityCrudHookTest.php
Situation without the patch applied:

+++ b/core/modules/system/src/Tests/Entity/EntityCrudHookTest.php
@@ -449,8 +449,8 @@ public function testTaxonomyVocabularyHooks() {
    $GLOBALS['entity_crud_hook_test'] = array();

Situation with the patch from comment #141 applied:

+++ b/core/modules/system/src/Tests/Entity/EntityCrudHookTest.php
@@ -449,8 +449,8 @@ public function testTaxonomyVocabularyHooks() {
    $_SESSION['entity_crud_hook_test'] = array();

Both filijonka and me think that that change will result in:

I think that alexpott will not commit the last block, it better a separate issue.

Hoping that this comment will clear things up. If not then can we chat about this on irc (#drupal-contribute). If I am not there you can also chat with filijonka about this.

ohhh sorry :(
My english not is so good :)
fixed!

ok looks good now, everything is protected and those that are accessed outside a test have a get/set function.

The meta issue has the beta evaluation.

Committed abe83bf and pushed to 8.0.x. Thanks!

diff --git a/core/modules/forum/src/Tests/ForumTest.php b/core/modules/forum/src/Tests/ForumTest.php
old mode 100755
new mode 100644
diff --git a/core/modules/migrate_drupal/src/Tests/d6/MigrateTaxonomyVocabularyTest.php b/core/modules/migrate_drupal/src/Tests/d6/MigrateTaxonomyVocabularyTest.php
old mode 100755
new mode 100644
diff --git a/core/modules/system/system.api.php b/core/modules/system/system.api.php
old mode 100755
new mode 100644
diff --git a/core/modules/taxonomy/src/Entity/Vocabulary.php b/core/modules/taxonomy/src/Entity/Vocabulary.php
old mode 100755
new mode 100644
diff --git a/core/modules/taxonomy/src/Tests/TermFieldTest.php b/core/modules/taxonomy/src/Tests/TermFieldTest.php
old mode 100755
new mode 100644
diff --git a/core/modules/taxonomy/src/Tests/TokenReplaceTest.php b/core/modules/taxonomy/src/Tests/TokenReplaceTest.php
old mode 100755
new mode 100644
diff --git a/core/modules/taxonomy/src/Tests/VocabularyCrudTest.php b/core/modules/taxonomy/src/Tests/VocabularyCrudTest.php
old mode 100755
new mode 100644
diff --git a/core/modules/taxonomy/src/Tests/VocabularyUiTest.php b/core/modules/taxonomy/src/Tests/VocabularyUiTest.php
old mode 100755
new mode 100644
diff --git a/core/modules/taxonomy/src/VocabularyForm.php b/core/modules/taxonomy/src/VocabularyForm.php
old mode 100755
new mode 100644
diff --git a/core/modules/taxonomy/src/VocabularyInterface.php b/core/modules/taxonomy/src/VocabularyInterface.php
old mode 100755
new mode 100644
diff --git a/core/modules/taxonomy/taxonomy.tokens.inc b/core/modules/taxonomy/taxonomy.tokens.inc
old mode 100755
new mode 100644
diff --git a/core/modules/views/src/Tests/ViewsTaxonomyAutocompleteTest.php b/core/modules/views/src/Tests/ViewsTaxonomyAutocompleteTest.php
old mode 100755
new mode 100644

Fixed the file modes on commit.