CommentFileSizeAuthor
#89 1818560-termng-89.patch8.46 KBandypost
#61 d8_term_ng.patch146.49 KBfago
#61 d8_termng_interdiff.txt2.11 KBfago
#57 drupal-1818560-57.patch146.17 KBdawehner
#57 interdiff.txt1.09 KBdawehner
#55 drupal-1818560-55.patch146.08 KBdawehner
#52 taxonomy-term-ng-1818560-52.patch145.76 KBBerdir
#52 taxonomy-term-ng-1818560-52-interdiff.txt2.27 KBBerdir
#47 taxonomy-term-ng-1818560-47.patch144.54 KBBerdir
#47 taxonomy-term-ng-1818560-47-interdiff.txt1.67 KBBerdir
#40 taxonomy-term-ng-1818560-40.patch150.73 KBjessebeach
#39 taxonomy-term-ng-1818560-39.patch146.65 KBBerdir
#37 taxonomy-term-ng-1818560-37.patch146.98 KBjessebeach
#37 EntityReferenceAutocompleteTest.rej_.txt3.37 KBjessebeach
#33 taxonomy-term-ng-1818560-33.patch149.92 KBWim Leers
#33 interdiff.txt2.56 KBWim Leers
#31 taxonomy-term-ng-1818560-31.patch150.1 KBYesCT
#31 interdiff-27-31.txt2.48 KBYesCT
#27 taxonomy-term-ng-1818560-27.patch150.11 KBBerdir
#27 taxonomy-term-ng-1818560-27-interdiff.txt2.05 KBBerdir
#24 taxonomy-term-ng-1818560-24.patch148.06 KBBerdir
#24 taxonomy-term-ng-1818560-24-interdiff.txt7.82 KBBerdir
#20 taxonomy-term-ng-1818560-20.patch146.87 KBBerdir
#20 taxonomy-term-ng-1818560-20-interdiff.txt1.26 KBBerdir
#19 d8_instance.patch.txt67.88 KBfago
#16 taxonomy-term-ng-1818560-16.patch146.91 KBBerdir
#14 taxonomy-term-ng-1818560-14.patch148.3 KBBerdir
#14 taxonomy-term-ng-1818560-14-interdiff.txt11.92 KBBerdir
#10 taxonomy-term-ng-1818560-10.patch145.76 KBBerdir
#10 taxonomy-term-ng-1818560-10-interdiff.txt52.77 KBBerdir
#7 taxonomy-term-ng-1818560-6.patch110.08 KBBerdir
#7 taxonomy-term-ng-1818560-6-interdiff.txt789 bytesBerdir
#5 taxonomy-term-ng-1818560-4.patch110.08 KBBerdir
#3 taxonomy-term-ng-1818560-3.patch110.85 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek’s picture

Issue tags: +Entity system, +D8MI

I think this is also D8MI related because it'll be necessary in order to translate Taxonomy terms, right? Tagging for now...

chx’s picture

Assigned: Unassigned » chx
Berdir’s picture

Status: Active » Needs review
FileSize
110.85 KB

So, @chx and me managed to start on this on parallel. Here's my patch, doing a full conversion, not using the BC decorator.

Most taxonomy tests are passing, even some fun ones like taxonomy term antonyms. Not completely through yet with everything.

- Changed the class and controllers to NG, defined properties.
- Changed all ->tid to ->id(), there are probably some wrong ones in there remaining, especially in forum.module.
- Some more ->name to ->label() changes that we missed originally
- After that, there's not much else other than description and format (which we want to combine in a single property later I think but that can probably be done in a follow-up as it will also require changes in the storage)
- Included the fix from #1862758: [Followup] Implement entity access API for terms and vocabularies that is now required as we need the bundle.
- taxonomy_get_tree() becomes an even bigger mess. We really need some sort of partial/lazy loading there.

Work in progress patch for @chx :)

Status: Needs review » Needs work

The last submitted patch, taxonomy-term-ng-1818560-3.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
110.08 KB

Ah, forgot a debug in there.

Status: Needs review » Needs work

The last submitted patch, taxonomy-term-ng-1818560-4.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
789 bytes
110.08 KB

And fixed that id(). Let's see if there are any more of those.

Status: Needs review » Needs work

The last submitted patch, taxonomy-term-ng-1818560-6.patch, failed testing.

Berdir’s picture

Assigned: chx » Berdir
Berdir’s picture

Status: Needs work » Needs review
FileSize
52.77 KB
145.76 KB

Had a lot of fun with forum.module, taxonomy_get_tree() and taxonomy terms re-ordering today. Still some broken things there, mostly related to parent handling. There's a mix with ->parent and ->parents which is used by forum.module and I don't see through that completely yet.

Status: Needs review » Needs work

The last submitted patch, taxonomy-term-ng-1818560-10.patch, failed testing.

Berdir’s picture

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.phpundefined
@@ -241,9 +241,7 @@ function testAddOrphanTopic() {
     $tree = taxonomy_get_tree($vid);
-    foreach ($tree as $term) {
-      taxonomy_term_delete($term->tid);
-    }
+    taxonomy_term_delete_multiple(array_keys(taxonomy_term_load_multiple(NULL)));

Can remove the get_tree() call here.

+++ b/core/modules/system/tests/modules/taxonomy_test/taxonomy_test.moduleundefined
@@ -11,13 +11,25 @@
+function taxonomy_test_entity_field_info_alter(&$info, $entity_type) {
+  $info['definitions']['antonym'] = array(
+    'label' => t('Antonym'),
+    'description' => t('The term antonym.'),
+    'type' => 'string_field',
+    'computed' => TRUE,

While this actually worked surprisingly easily I'm not sure if we should be doing that. We have separate tests now to make sure that the alter hooks work as part of the entity tests (for various entity types) and this should be done using a field, not custom glue code.

I'd suggest to keep it here an then open a follow-up to remove that (probably the whole module I don't think there's anything else there) and the corresponding test.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.phpundefined
@@ -140,7 +140,7 @@ function value_form(&$form, &$form_state) {
-            $choice->option = array($term->tid => str_repeat('-', $term->depth) . $term->name);
+            $choice->option = array($term->id() => str_repeat('-', $term->depth) . $term->name);

We seem to be doing this in various, inconsistent ways, should probably add a helper function for generating this array.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.phpundefined
@@ -158,7 +158,7 @@ function value_form(&$form, &$form_state) {
         foreach ($result as $term) {
-          $options[$term->tid] = $term->name;
+          $options[$term->id()] = $term->name;

@@ -320,7 +320,7 @@ function validate_term_strings(&$form, $values) {
     foreach ($result as $term) {
       unset($missing[strtolower($term->name)]);
-      $tids[] = $term->tid;
+      $tids[] = $term->id();

@@ -360,7 +360,7 @@ public function adminSummary() {
       foreach ($result as $term) {
-        $this->value_options[$term->tid] = $term->name;
+        $this->value_options[$term->id()] = $term->name;

This is a direct query, shouldn't use id()

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -25,10 +25,6 @@ class TermStorageController extends DatabaseStorageController {
-    // Save new terms with no parents by default.
-    if (!isset($entity->parent)) {
-      $entity->parent = array(0);

@@ -87,22 +83,20 @@ protected function postDelete($entities) {
-    if (isset($entity->parent)) {

I think I need to keep this, instead of the default value, this currently forces it to 0 and therefore deletes parents when it's not updated.

Or actually make it a properly computed field and load the parents when saving. Not so cool for performance, though, especially when saving a lot of terms (weight reordering)

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -652,7 +665,7 @@ function taxonomy_term_load_parents_all($tid) {
@@ -776,26 +789,27 @@ function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities

Wondering if we should move this function into the storage controller then it could be switched with a different implementation easily.

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -19,21 +19,17 @@ function taxonomy_term_page(Term $term) {
-  // Build breadcrumb based on the hierarchy of the term.
-  $current = (object) array(
-    'tid' => $term->tid,
...
+  while ($parents = taxonomy_term_load_parents($term->id())) {
+    $term = array_shift($parents);
+    $breadcrumb[] = l($term->label(), 'taxonomy/term/' . $term->id());

I broke this, overwriting the term now.

fago’s picture

Amazing progress. berdir++

Wondering if we should move this function into the storage controller then it could be switched with a different implementation easily.

Yes. I think that any access to the storage should go via the storage controller or EFQ. IF you need to make custom queries, add a method to the storage controller. Properly done, you'd have to add a new interface also.

I think I need to keep this, instead of the default value, this currently forces it to 0 and therefore deletes parents when it's not updated.

I'm not sure I got this, let's discuss on IRC.

Berdir’s picture

Status: Needs work » Needs review
FileSize
11.92 KB
148.3 KB

- Fixed some of the things pointed out above
- Added a PathItem for path.module.
- Fixed parent handling, back to only setting it to 0 for new terms explicitly and only changing it if it's not NULL. What's a bit strange there is that I had a case where the first offset was 1, not 0, the same as the term id. I think that's because the select values are directly passed to setValue(), wondering if we should default to re-order the items? I think field API does that too.

All failing tests above were green for me...

Status: Needs review » Needs work

The last submitted patch, taxonomy-term-ng-1818560-14.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
146.91 KB

Re-rolled, can't see anything different in the patch other then the removed debug that I forgot to remove before.

Status: Needs review » Needs work

The last submitted patch, taxonomy-term-ng-1818560-16.patch, failed testing.

fago’s picture

double-post

fago’s picture

FileSize
67.88 KB

and the wrong issue... *ignore pls*

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.26 KB
146.87 KB

This should fix the last test fails, mad a rather dumb mistake with that breadcrumb thing ;)

dawehner’s picture

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.phpundefined
@@ -240,10 +240,7 @@ function testForum() {
+    taxonomy_term_delete_multiple(array_keys(entity_load_multiple_by_properties('taxonomy_term', array('vid' => $vid))));

Just wondering whether the storage controller should be accessed directly?

+++ b/core/modules/path/lib/Drupal/path/Type/PathItem.phpundefined
@@ -0,0 +1,43 @@
+  static $propertyDefinitions;

Static, but probably protected? This is also not done in other places of FieldItemBase, so we can't change it here? (Just curious).

+++ b/core/modules/path/path.moduleundefined
@@ -235,13 +235,13 @@ function path_form_taxonomy_term_form_alter(&$form, $form_state) {
+    $path = ($term->id() ? drupal_container()->get('path.crud')->load(array('source' => 'taxonomy/term/' . $term->id())) : array());

This could use Drupal::service.

+++ b/core/modules/path/path.moduleundefined
@@ -235,13 +235,13 @@ function path_form_taxonomy_term_form_alter(&$form, $form_state) {
-      'source' => isset($term->tid) ? 'taxonomy/term/' . $term->tid : NULL,
+      'source' => $term->id() ? 'taxonomy/term/' . $term->id() : NULL,

@@ -265,18 +265,45 @@ function path_form_taxonomy_term_form_alter(&$form, $form_state) {
+      $source = 'taxonomy/term/' . $term->id();

@@ -286,19 +313,18 @@ function path_taxonomy_term_insert(Term $term) {
+      $source = 'taxonomy/term/' . $term->id();

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/TaxonomyAttributesTest.phpundefined
@@ -35,13 +35,13 @@ public static function getInfo() {
+    $term_uri = url('taxonomy/term/' . $term->id(), array('absolute' => TRUE));
...
+    $parser->parse($graph, $this->drupalGet('taxonomy/term/' . $term->id()), 'rdfa', $base_uri);

+++ b/core/modules/rdf/rdf.moduleundefined
@@ -767,7 +767,7 @@ function rdf_preprocess_taxonomy_term(&$variables) {
+        'about' => url('taxonomy/term/' . $term->id()),

A potential follow up to use $term->uri(). Not sure about the other two cases below.

+++ b/core/modules/path/path.moduleundefined
@@ -265,18 +265,45 @@ function path_form_taxonomy_term_form_alter(&$form, $form_state) {
+      drupal_container()->get('path.crud')->save($source, $term->path->alias, $langcode);

@@ -286,19 +313,18 @@ function path_taxonomy_term_insert(Term $term) {
+      drupal_container()->get('path.crud')->delete(array('pid' => $term->path->pid));
...
+      drupal_container()->get('path.crud')->save($source, $term->path->alias, $langcode, $pid);

@@ -308,7 +334,7 @@ function path_taxonomy_term_update(Term $term) {
+  drupal_container()->get('path.crud')->delete(array('source' => 'taxonomy/term/' . $term->id()));

Drupal::service()

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -111,17 +111,30 @@ class Term extends Entity implements ContentEntityInterface {
+  protected $values = array(

Do we have a standard how to document overridden properties?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -111,17 +111,30 @@ class Term extends Entity implements ContentEntityInterface {
+  protected function init() {
+    parent::init();
+    unset($this->tid);
+    unset($this->uuid);
+    unset($this->vid);
+    unset($this->name);
+    unset($this->weight);
+    unset($this->format);
+    unset($this->description);
+    unset($this->parent);
   }

Would be cool to explain why these values are unset. This might be not trivial for people not knowing EntityNG etc.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/entity_reference/selection/TermSelection.phpundefined
@@ -67,7 +67,7 @@ public function getReferencableEntities($match = NULL, $match_operator = 'CONTAI
+            $options[$vocabulary->id()][$term->id()] = str_repeat('-', $term->depth) . check_plain($term->name);

Shouldn't this be $term->name->value now?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -24,11 +24,10 @@ class TermStorageController extends DatabaseStorageController {
+    if (empty($values['parent'])) {
+      $values['parent'] = array(0);
     }
+    $entity = parent::create($values);

This wouldn't allow people to use hook_entity_create and set a default parent value, I guess that's okay?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -87,9 +86,14 @@ protected function postDelete($entities) {
+    if (isset($parent['value']) && $parent['value'] !== NULL) {

Doesn't isset() already return FALSE, if it's NULL? It has been different on array_key_exists

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -97,8 +101,8 @@ protected function postSave(EntityInterface $entity, $update) {
+          'parent' => (int)$parent->value,

Nitpick alarm: Missing space after "(int)".

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/HooksTest.phpundefined
@@ -53,29 +53,29 @@ function testTaxonomyTermHooks() {
+    taxonomy_term_delete($term->id());

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/LoadMultipleTest.phpundefined
@@ -52,20 +52,20 @@ function testTaxonomyTermMultipleLoad() {
+    taxonomy_term_delete($deleted->id());

Why not simply call $term->delete() ?

Berdir’s picture

Issue tags: +sprint

Tagging.

Gábor Hojtsy’s picture

Issue tags: +language-content

Tagging for D8MI topic language-content.

Berdir’s picture

Thanks for the review! Originally thought to wait with this on the ER taxonomy reference issue but it looks like that one is a bit blocked. So let's continue here.

- Used Drupal::entityQuery() to load the terms based on the vid.
- Did not change PathItem. I'm actually not sure what that thing isn't defined in the base class and I'd like to keep it consistent until we do.
- Changed to Drupal::service()
- Documented $values
- The init() thing is described in the parent implementation a bit. Not sure about duplicating that everywhere. And it might go away because those properties might go away if we switch to interfaces (it will be useless then, we only have the public properties for IDE integration).
- "Shouldn't this be $term->name->value now?" taxonomy_get_tree() in some cases still returns stdClasses. Will probably have to get rid of that and instead hope for lazy loading entity classes to happen.
- Removed unecessary NULL check, you're right.
- Changed to ->delete(), those were global search & replaces.

Berdir’s picture

Also created the following related/follow-up issues:

- #1962806: Remove Drupal\taxonomy\Tests\HooksTest and taxonomy_test.module
- #1962808: Move static $propertyDefinitions into FieldItemBase class

Both can be done before or after this issue and are nice Novice issues I think.

Status: Needs review » Needs work

The last submitted patch, taxonomy-term-ng-1818560-24.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
150.11 KB

This should fix that test.

YesCT’s picture

jibran’s picture

klausi’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.php
@@ -109,4 +113,60 @@ public function resetCache(array $ids = NULL) {
+  /**
+   * Overrides \Drupal\Core\Entity\DataBaseStorageControllerNG::basePropertyDefinitions().
+   */
+  public function baseFieldDefinitions() {

Comment does not match function name.

YesCT’s picture

Observations (nothing wrong)

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

there are many ->name to ->label
and ->name to ->name->value

+++ b/core/modules/forum/forum.moduleundefined
@@ -781,7 +782,7 @@ function forum_forum_load($tid = NULL) {
-    $forums[$forum->tid] = $forum;
+    $forums[$forum->id()] = $forum;

there are many $forum->tid to $forum->id()

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -412,28 +405,17 @@ function taxonomy_overview_terms_submit($form, &$form_state) {
-      $term['parent'] = $term['parents'][0];
-      $term['weight'] = $weight;
...
+      $term->parent->value = $term->parent->value;
+      $term->weight->value = $weight;

another common substitution pattern. $term['parent'] to $term->parent->value and weight too.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTest.phpundefined
@@ -109,31 +109,31 @@ function testTaxonomyNode() {
-    $this->assertText($term1->name, 'Term is displayed when editing the node.');
+    $this->assertText($term1->label(), 'Term is displayed when editing the node.');

$term->name sometimes is $term->label() and sometimes $term->label?

-------------------
patch coming for:

+++ b/core/modules/forum/forum.admin.incundefined
@@ -5,31 +5,37 @@
+ *   (optional) A forum or container term to be
+ *   edited. Defaults to NULL

http://drupal.org/node/1354#drupal

Needs a period at the end of the sentence.
"All documentation and comments should form proper sentences, use proper grammar and punctuation,"

this can be wrapped more.

"Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over..."

Also, Defaults to...
was clarified to be extra. I'm leaving it in though for consistency within the patch. #1916652: defaults to in 1354 doc standards for optional args

Question 1:

+++ b/core/modules/path/lib/Drupal/path/Type/PathItem.phpundefined
@@ -0,0 +1,43 @@
+   * @var array
+   */
+  static $propertyDefinitions;

does this need to specify public vs protected?

+++ b/core/modules/path/lib/Drupal/path/Type/PathItem.phpundefined
@@ -0,0 +1,43 @@
+  }
+}

adding newline before class closing }

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -109,4 +113,60 @@ public function resetCache(array $ids = NULL) {
+    // @todo: Combine with description.

just @todo (not @todo:)
http://drupal.org/node/1354#todo

Question 2: Does this need an issue? Is it blocking? does this depend on anything else?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/EfqTest.phpundefined
@@ -69,6 +69,6 @@ function testTaxonomyEfq() {
-    $this->assertEqual($term->tid, $tid, 'Taxonomy term can be created based on the IDs');
+    $this->assertEqual($term->id(), $tid, 'Taxonomy term can be created based on the IDs');

maybe a novice follow-up to look for other asserts where the punctuation is missing from sentences.

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -282,17 +276,17 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
-      'edit' => array('title' => t('edit'), 'href' => 'taxonomy/term/' . $term->tid . '/edit', 'query' => $destination),
-      'delete' => array('title' => t('delete'), 'href' => 'taxonomy/term/' . $term->tid . '/delete', 'query' => $destination),
+      'edit' => array('title' => t('edit'), 'href' => 'taxonomy/term/' . $term->id() . '/edit', 'query' => $destination),
+      'delete' => array('title' => t('delete'), 'href' => 'taxonomy/term/' . $term->id() . '/delete', 'query' => $destination),

check and make sure these are slotted already with an issue to change the case to Edit and Delete...
I quickly looked at #421118: [Meta] Standardize capitalization on actions I think we need a new sub-issue on that meta for taxonomy/term operations in the taxonomy module.
Not blocking this issue. It's unrelated to this patch.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -369,6 +369,19 @@ function taxonomy_menu() {
+ * Access callback for creating a new taxonomy term.
...
+function taxonomy_term_create_access(Vocabulary $vocabulary) {

check access callback standards.
http://drupal.org/coding-standards/docs#menu-callback
"In the first line, declare what type of callback it is (Page callback, Access callback, etc.), and give a short description of what it does (like you would for any function). Total line length: 80 characters, ending in "."
At the end, include an @see line that points to the hook_menu() implementation where this callback is registered. If multiple hook implementations use the callback, include one @see line for each."

Question 3:
I can't figure out what the @see might be. I dont see a hook_create_access(). Maybe taxonomy_menu() ?

--

About #30:
core/modules/node/lib/Drupal/node/NodeStorageController.php

 /**
   * Overrides \Drupal\Core\Entity\DataBaseStorageControllerNG::basePropertyDefinitions().
   */
  public function baseFieldDefinitions() {

Hmm. It's spelled Database.. not DataBase..
is it worth a follow-up to fix that everywhere? (not this issue's fault)

For now, based on line 727 of core/lib/Drupal/Core/Entity/DatabaseStorageController.php:

  /**
   * Defines the base properties of the entity type.
   *
   * @todo: Define abstract once all entity types have been converted.
   */
  public function baseFieldDefinitions() {

I changed it to

   * Overrides \Drupal\Core\Entity\DatabaseStorageControllerNG::baseFieldDefinitions().
Berdir’s picture

This is already part of a critical meta issue, there's not much point in making this major too :)

Wim Leers’s picture

Issue tags: +Spark
FileSize
2.56 KB
149.92 KB

#31:

+++ b/core/modules/path/lib/Drupal/path/Type/PathItem.phpundefined
@@ -0,0 +1,43 @@
+   * @var array
+   */
+  static $propertyDefinitions;

does this need to specify public vs protected?

AFAICT this is implicitly answered by #1962808: Move static $propertyDefinitions into FieldItemBase class: we shouldn't change it here, because many other FieldItemBase subclasses do it the exact same way. We should stick to "the standard" (insofar as it is one) and change all of them at once, if any.
This has been pointed out & answered before already.

RE: "novice follow-up to fix punctuation": done, #1965510: Fix punctuation in Taxonomy module tests' assertion sentences.

Question 3: fixed.


#32: This issue already is major :)


(I'm coming at this from the angle "let's make in-place editing also work on non-Node entities, e.g. Taxonomy Terms. Since Edit already depends on EntityNG, and Taxonomy Terms are not yet EntityNG, I can't work on that without this issue being solved.)

First: I cannot find any nitpicks… that must be a first :D

Second, code review:

+++ b/core/includes/entity.incundefined
@@ -792,13 +792,23 @@ function entity_page_access(EntityInterface $entity, $operation = 'view') {
-function entity_page_create_access($entity_type) {
+function entity_page_create_access($entity_type, $bundle = NULL) {
+  $definition = drupal_container()->get('plugin.manager.entity')->getDefinition($entity_type);
+
+  // Pass in the entity bundle if given and required.
+  $values = array();
+  if ($bundle && isset($definition['entity_keys']['bundle'])) {
+    $values[$definition['entity_keys']['bundle']] = $bundle;

We're changing this function's signature, but in the function description it says this:

 * Some entity types might have create access per bundle or something else.
 * In that case you have to create a custom access callback.

Shouldn't that be updated as well? I'm not sure how to update it correctly, because it is currently pretty confusing: comments and code somewhat contradict each other.

+++ b/core/modules/forum/forum.moduleundefined
@@ -1042,8 +1043,8 @@ function template_preprocess_forum_list(&$variables) {
-    $variables['forums'][$id]->description = !empty($forum->description) ? filter_xss_admin($forum->description) : '';
-    $variables['forums'][$id]->link = url("forum/$forum->tid");
+    $variables['forums'][$id]->description = check_markup($forum->description->value, $forum->format->value);
+    $variables['forums'][$id]->link = url('forum/' . $forum->id());

The handling of description is changed significantly here. Is that really correct? I do see further in the patch that for taxonomy.module, check_markup() must be used, but if forum.module used filter_xss_admin() before, then we should probably continue to do so.

+++ b/core/modules/options/options.api.phpundefined
index 0000000..f853d88
--- /dev/null

--- /dev/null
+++ b/core/modules/path/lib/Drupal/path/Type/PathItem.phpundefined

Do we really need to introduce this in this patch? It seems like a change that is only tangentially related to what's the core goal here?
We do need this, because we need the Taxonomy Term "extra fields" to be registered as proper Entity Properties, and then this is necessary.
This is ok.

+++ b/core/modules/path/path.moduleundefined
@@ -265,18 +265,45 @@ function path_form_taxonomy_term_form_alter(&$form, $form_state) {
+function path_entity_field_info($entity_type) {
+  if ($entity_type == 'taxonomy_term') {

A conditionally existing entity field? That seems very bizarre?
However bizarre it may be, it seems that this is correct. Path module alters Taxonomy module to allow a custom alias for each term to be set right in the term's form. Hence it must also update the list of Entity Properties, and that's what this does.
It feels very dirty to be modifying Taxonomy Term's Entity Properties from another module, but that's how this has worked for quite some time, this is not the appropriate issue to change that.
This is ok.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -108,21 +108,39 @@ class Term extends Entity implements ContentEntityInterface {
+    'langcode' => array(LANGUAGE_DEFAULT => array(0 => array('value' => LANGUAGE_NOT_SPECIFIED))),

One is "default', other is 'not specified'. They've currently got identical values, but I wonder if this is conceptually correct.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -24,11 +24,10 @@ class TermStorageController extends DatabaseStorageController {
-    $entity = parent::create($values);
-    // Save new terms with no parents by default.
-    if (!isset($entity->parent)) {
-      $entity->parent = array(0);
+    if (empty($values['parent'])) {
+      $values['parent'] = array(0);
     }
+    $entity = parent::create($values);

1) Previously, the parent method was called first. Now it's called last. Why?
2) The comment was removed. Restored.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -142,7 +142,7 @@ function taxonomy_entity_bundle_info() {
@@ -359,8 +359,8 @@ function taxonomy_menu() {

@@ -359,8 +359,8 @@ function taxonomy_menu() {
     'title' => 'Add term',
     'page callback' => 'taxonomy_term_add',
     'page arguments' => array(3),
-    'access callback' => 'entity_page_create_access',
-    'access arguments' => array('taxonomy_term'),
+    'access callback' => 'taxonomy_term_create_access',
+    'access arguments' => array(3),
     'type' => MENU_LOCAL_ACTION,
     'file' => 'taxonomy.admin.inc',
   );
@@ -369,6 +369,19 @@ function taxonomy_menu() {

@@ -369,6 +369,19 @@ function taxonomy_menu() {
 }
 
 /**
+ * Access callback: Creates a new taxonomy term.
+ *
+ * @param \Drupal\taxonomy\Plugin\Core\Entity\Vocabulary $vocabulary
+ *   The vocabulary entity that represents the term bundle.
+ *
+ * @return bool
+ *   TRUE if access is allowed, FALSE if not.
+ */
+function taxonomy_term_create_access(Vocabulary $vocabulary) {
+  return entity_page_create_access('taxonomy_term', $vocabulary->id());
+}

I was going to question whether it is necessary to create an alternative to entity_page_create_access(), but sadly, it is. This is fine.


Finally: manual testing found no problems, with one big exception. I have added an image field to my Taxonomy Terms, and I can still create and edit terms with images, but, upon rendering, the image is not rendered correctly.

Before:
(on the term page)

<div id="taxonomy-term-3" class="taxonomy-term vocabulary-tags">
  <div class="content">
    <div class="field field-name-field-photo field-type-image field-label-above">
      <div class="field-label">Photo</div>
      <div class="field-items">
        <div class="field-item even"><img typeof="foaf:Image" src="http://localhost/patchtest/sites/default/files/beta.png" width="834" height="523" alt=""></div>
      </div>
    </div>
  </div>
</div>

(in a view)

<div class="views-field views-field-field-photo">
  <span class="views-label views-label-field-photo">Photo: </span>    
  <div class="field-content"><img typeof="foaf:Image" src="http://localhost/patchtest/sites/default/files/beta.png" width="834" height="523" alt=""></div>
</div>

After:
(on term page)

<div id="taxonomy-term-3" class="taxonomy-term vocabulary-tags">
  <div class="content">
    <div class="field field-name-field-photo field-type-image field-label-above">
      <div class="field-label">Photo</div>
      <div class="field-items">
        <div class="field-item even"><img typeof="foaf:Image" src="http://localhost/patchtest/" width="834" height="523" alt=""></div>
      </div>
    </div>
  </div>
</div>

(in a view)

<div class="views-field views-field-field-photo">
  <span class="views-label views-label-field-photo">Photo: </span>    
  <div class="field-content"></div>
</div>

When looking at $item in theme_image_formatter() (on the term page, because in a view it doesn't even get to that function:

Before:

    Array
(
    [fid] => 1
    [alt] => 
    [title] => 
    [width] => 834
    [height] => 523
    [uuid] => 37c73b3a-c8f9-4323-ac08-8c50f3d123db
    [langcode] => und
    [uid] => 1
    [filename] => beta.png
    [uri] => public://beta.png
    [filemime] => image/png
    [filesize] => 717359
    [status] => 1
    [timestamp] => 1365514289
    [*entityType] => file
    [*enforceIsNew] => 
    [*newRevision] => 
    [*isDefaultRevision] => 1
    [rdf_mapping] => Array
        (
        )

)

After:
(on the term page

    Array
(
    [fid] => 1
    [alt] => 
    [title] => 
    [width] => 834
    [height] => 523
)

Nodes are also EntityNG, and there this problem does not occur (an image field there works fine).

Berdir’s picture

Will need to look at the image problem. Wondering if it has to do with #1957748: hook_field_prepare_view() is passed empty field values for EntityNG Entities.

The access related part are from #1862758: [Followup] Implement entity access API for terms and vocabularies, they are not really related to this issue but required for it.

Wim Leers’s picture

#34: I tested the first patch in the first issue you linked to, unfortunately it causes further breakage: #1957748-20: hook_field_prepare_view() is passed empty field values for EntityNG Entities.

Gábor Hojtsy’s picture

One more D8Mi tag.

jessebeach’s picture

The patch in #33 doesn't apply since (8320fdc2d55b2468b09d1246ee8f03224322ee2d).

The file EntityReferenceAutocompleteTest.php was updated to reflect the use of entities for taxonomy terms.

It appears that the changes to this file are no longer necessary. I've updated the patch and attached the git-apply rej file to show what I left out.

Status: Needs review » Needs work

The last submitted patch, taxonomy-term-ng-1818560-37.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
146.65 KB

Thanks for the re-roll, yes looks like that is no longer required. Also fixed a conflict in EfqTest that you overlooked and a new one in forum.admin.inc.

jessebeach’s picture

This is a tough one to keep up to date!

Reroll.

Wim Leers’s picture

I believe the problem I encountered and explained at length at the bottom of #33 is solved by #1957748: hook_field_prepare_view() is passed empty field values for EntityNG Entities?

Wim Leers’s picture

Issue summary: View changes

added follow-up issue section

Wim Leers’s picture

Status: Needs review » Needs work

Testing this again; I wanted to check if my understanding in #41 is correct, and it is :)

This patch functions absolutely fine, but there is a bug in Field API for EntityNG handling, which is fixed by #1957748: hook_field_prepare_view() is passed empty field values for EntityNG Entities.

BUT there are a few small things to be fixed or clarified in this patch, that I noted in #33. Here are the things that still need to be fixed:

+++ b/core/includes/entity.incundefined
@@ -792,13 +792,23 @@ function entity_page_access(EntityInterface $entity, $operation = 'view') {
-function entity_page_create_access($entity_type) {
+function entity_page_create_access($entity_type, $bundle = NULL) {
+  $definition = drupal_container()->get('plugin.manager.entity')->getDefinition($entity_type);
+
+  // Pass in the entity bundle if given and required.
+  $values = array();
+  if ($bundle && isset($definition['entity_keys']['bundle'])) {
+    $values[$definition['entity_keys']['bundle']] = $bundle;

We're changing this function's signature, but in the function description it says this:

 * Some entity types might have create access per bundle or something else.
 * In that case you have to create a custom access callback.

Shouldn't that be updated as well? I'm not sure how to update it correctly, because it is currently pretty confusing: comments and code somewhat contradict each other.

+++ b/core/modules/forum/forum.moduleundefined
@@ -1042,8 +1043,8 @@ function template_preprocess_forum_list(&$variables) {
-    $variables['forums'][$id]->description = !empty($forum->description) ? filter_xss_admin($forum->description) : '';
-    $variables['forums'][$id]->link = url("forum/$forum->tid");
+    $variables['forums'][$id]->description = check_markup($forum->description->value, $forum->format->value);
+    $variables['forums'][$id]->link = url('forum/' . $forum->id());

The handling of description is changed significantly here. Is that really correct? I do see further in the patch that for taxonomy.module, check_markup() must be used, but if forum.module used filter_xss_admin() before, then we should probably continue to do so.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -108,21 +108,39 @@ class Term extends Entity implements ContentEntityInterface {
+    'langcode' => array(LANGUAGE_DEFAULT => array(0 => array('value' => LANGUAGE_NOT_SPECIFIED))),

One is "default', other is 'not specified'. They've currently got identical values, but I wonder if this is conceptually correct.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -24,11 +24,10 @@ class TermStorageController extends DatabaseStorageController {
-    $entity = parent::create($values);
-    // Save new terms with no parents by default.
-    if (!isset($entity->parent)) {
-      $entity->parent = array(0);
+    if (empty($values['parent'])) {
+      $values['parent'] = array(0);
     }
+    $entity = parent::create($values);

Previously, the parent method was called first. Now it's called last. Why?

Almost there! :)

Berdir’s picture

- The entity_page_create_access() related stuff should be discussed/added to #1862758: [Followup] Implement entity access API for terms and vocabularies, if we need to change it, it needs to happen there.
- the description thing in forum.module is IMHO a bug. it's the term description, and the term description is supposed to be rendered with the associated term description format. Not sure if this is even security related, in fact...
- The language default value is copied from Comment.php, where it is exactly the same. I think this is correct like this. I don't fully understand this, actually :)
- The difference is that it is now easier to change $values before passing it to parent::create() instead of altering the generated entity

Wim Leers’s picture

3 out of the 4 answers are satisfactory.

- the description thing in forum.module is IMHO a bug. it's the term description, and the term description is supposed to be rendered with the associated term description format. Not sure if this is even security related, in fact...

Yet when you look at admin/structure/forum/edit/forum/4, it is impossible to select a text format! Not before, not after the patch. IMHO this change should be reverted and if we want to change this, it should be in a separate issue.

fago’s picture

I had another look and I must say that this looks already pretty good. Here are some comments:

+++ b/core/modules/path/path.module
@@ -262,18 +262,45 @@ function path_form_taxonomy_term_form_alter(&$form, $form_state) {
+      'type' => 'path_field',
+      'label' => t('The path alias'),
+      'computed' => TRUE,

Off-topic for this conversion, but I think that shows that we should allow computed fields to have insert/preSave/update methods. See related discussion over at #1969728: Implement Field API "field types" as TypedData Plugins.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.php
@@ -46,7 +46,7 @@ public function form(array $form, array &$form_state, EntityInterface $term) {
-      '#default_value' => $term->langcode,
+      '#default_value' => $term->language()->langcode,

Minor, but should be $term->langcode->value imho.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.php
@@ -78,9 +78,14 @@ protected function postDelete($entities) {
-    if (isset($entity->parent)) {
+    // Only change the parents if a value is set, keep the existing values if
+    // not. They are keyed by term id if set through the UI, so take the first
+    // value.
+    $parents = $entity->parent->getValue();
+    $parent = reset($parents);
+    if (isset($parent['value'])) {

Could that be replaced with if (isset($entity->parent[0]->value) ?

Gábor Hojtsy’s picture

Wim: looking at the pre-existing taxonomy schema, taxonomy terms have a format property as the format of their description. While forums don't expose the description as a field with format, they store as taxonomy terms (with a NULL value for format). When that is passed on to check_plain() the fallback format is used, which is somewhat right by default. I agree unless the UI is not modified on forums to allow for format specification (which would likely require some upgrade path, heh), we should avoid doing this.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
144.54 KB

Opened #1975066: Forum terms do not respect/expose term description format for the forum problem.

Re-roll, changing to $term->langcode->value.

Using parent[0] is not possible, unfortunately, see the comment above. When set through the form, the keys of that list are set based on the term id, so it might be $term->parent[1], or $term->parent[35764]. Not sure if Entity\Field.php should enforce 0,1,2,3... for the field item id's (delta in field api speech), possibly. But that seems like a separate issue.

The access callback got commited, anything left to prevent an RTBC here? :)

Status: Needs review » Needs work

The last submitted patch, taxonomy-term-ng-1818560-47.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

Once this passes tests again, I'll RTBC :)

It's a bizarre failure. Re-testing.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Entity system, +D8MI, +sprint, +language-content, +Spark, +Entity Field API, +language-content-property

The last submitted patch, taxonomy-term-ng-1818560-47.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.27 KB
145.76 KB

Ah, that's a new test, not so bizarre :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Woot! :) Thanks, Berdir! :)

fago’s picture

Awesome work!

When set through the form, the keys of that list are set based on the term id, so it might be $term->parent[1], or $term->parent[35764]. Not sure if Entity\Field.php should enforce 0,1,2,3... for the field item id's (delta in field api speech), possibly. But that seems like a separate issue.

I see. I'm not sure we can enforce that, but at least we should clean-up a mis-use like that. But yes - separate issue.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
146.08 KB

Some oddities I came up with while rerolling the patch:

  • The patch contains $term->parents while there is just $term->parent as far as I can tell
  • $term->depth is not defined on Term.php, which then leads to an inconsistent api: $term->depth vs. $term->parent->value
    is there a reason for that? (I couldn't find an answer in this issue)
+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -789,26 +789,27 @@ function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities
         unset($term->parent);
-        $term->parents = $parents[$vid][$term->tid];
+        $tid = $load_entities ? $term->id() : $term->tid;
+        $term->parents = $parents[$vid][$tid];
         $tree[] = $term;
-        if (!empty($children[$vid][$term->tid])) {
+        if (!empty($children[$vid][$tid])) {

That's one place where parents vs. parent comes up.

Status: Needs review » Needs work

The last submitted patch, drupal-1818560-55.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
146.17 KB

Fixed two small points.

One problem which currently exists is that $term->parent->value is empty, as the comment suggests. What is the proper alternative way to load it?

fago’s picture

+    // Only change the parents if a value is set, keep the existing values if
+    // not. They are keyed by term id if set through the UI, so take the first
+    // value.
+    $parents = $entity->parent->getValue();
+    $parent = reset($parents);
+    if (isset($parent['value'])) {

Hm, realized there is a another problem with $term->parent being keyed wrong. Instead of working around it at multiple places, shouldn't we better fix the UI to properly write the list? (also see #47)

The patch contains $term->parents while there is just $term->parent as far as I can tell

Indeed. That's quite a mess, but it's not introduced by that patch. So I think it's better to clean it up in another issue. $term->depth actually is something that differs by parent, so it would belong more into $term->parents[X]->depth.

fago’s picture

#57: drupal-1818560-57.patch queued for re-testing.

xjm’s picture

fago’s picture

FileSize
2.11 KB
146.49 KB

Updated the patch to fix $term->parent being keyed wrong. I figured the form controller overrides submit() instead of buildEntity() also, so fixed that as well.

Interdiff + updated patch attached.

dawehner’s picture

+++ b/core/modules/forum/forum.moduleundefined
@@ -722,16 +722,17 @@ function forum_forum_load($tid = NULL) {
-  $forum_term->parents = taxonomy_term_load_parents_all($forum_term->tid);
+  $forum_term->parents = taxonomy_term_load_parents_all($forum_term->id());

+++ b/core/modules/forum/forum.pages.incundefined
@@ -30,21 +30,21 @@ function forum_page($forum_term = NULL) {
   if ($forum_term->parents) {
     foreach (array_reverse($forum_term->parents) as $parent) {

@@ -69,7 +69,7 @@ function forum_page($forum_term = NULL) {
     '#parents' => $forum_term->parents,

I'm wondering whether we can use somehow ->parent here as well?

fago’s picture

Yes. But while it doesn't look so in the patch, when you dig through the code there are lots of ->parent vs ->parents usages left. As this has not been introduced here, I agree with berdir that it's better to postpone cleaning this up to another issue.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Sure I have seen that. Cool.

andypost’s picture

Only path module related changes seems out-scope. The same functionality needed for nodes so this issue require a follow-up to unify usage of path aliasing on nodes and terms

+++ b/core/modules/path/path.moduleundefined
@@ -262,18 +262,45 @@ function path_form_taxonomy_term_form_alter(&$form, $form_state) {
+ * Implements hook_data_type_info().
+ */
+function path_data_type_info() {
+  $info['path_field'] = array(
+    'label' => t('Path field item'),
+    'description' => t('An entity field containing a path alias and related data.'),
+    'class' => '\Drupal\path\Type\PathItem',
+    'list class' => '\Drupal\Core\Entity\Field\Type\Field',
+  );
+  return $info;
+}
+
+/**
+ * Implements hook_entity_field_info().
+ */
+function path_entity_field_info($entity_type) {
+  if ($entity_type === 'taxonomy_term') {
+    $info['definitions']['path'] = array(
+      'type' => 'path_field',
+      'label' => t('The path alias'),
+      'computed' => TRUE,
+      'list' => TRUE,
+    );
+    return $info;
+  }

Maybe better split this 'path-related' hunks to separate issue seems nodes needs this too

andypost’s picture

#61: d8_term_ng.patch queued for re-testing.

Berdir’s picture

The path part is required. I would not have done it here if it wouldn't be. Nodes are still using the BC mode, that's why that (and a million other changes) was not *yet* necessary.

Wim Leers’s picture

#65: what #67 says, and what I said in #33, where I initially thought the same:

Do we really need to introduce this in this patch? It seems like a change that is only tangentially related to what's the core goal here?
We do need this, because we need the Taxonomy Term "extra fields" to be registered as proper Entity Properties, and then this is necessary.
This is ok.

andypost’s picture

I just tell about follow-up to refactor this type-data
EDIT related issue for path as field #1751210: Convert URL alias form element into a field and field widget

xjm’s picture

Issue tags: +needs profiling
xjm’s picture

jhodgdon’s picture

Regarding the path change, can someone comment on whether Pathauto would still be able to function with this patch? Pathauto would need to be able to:
- Override the path editing area (add a "use automatic" checkbox)
- Override the node save behavior (create an automatic alias if the box is checked)

I'm just not clear on what this change means for the Path module and whether Pathauto would be able to do the same kinds of changes it has done with past versions of Drupal.

jhodgdon’s picture

Issue summary: View changes

added related issues which solve part of this

YesCT’s picture

Issue summary: View changes

added related issues that have been found while working on this. maybe could use some clarification if any are blocking this, or are truely follow-ups (postponsed on this)

YesCT’s picture

Issue tags: +Needs manual testing

updated issue summary.
Also adding tag to manually test #72

Berdir’s picture

Issue tags: -Needs manual testing

Yes, there will be various ways how that will still be possible. The only change here is that we make the properties on the entity explicit, the same form/storage alter hooks can still be used to define what actually happens with it.

If we, as suggested by @fago in #45, introduce methods for computed properties to act on save, then pathauto might even be able to simply alter the typed data definition and use a different class.

Berdir’s picture

Removing the tag was a crosspost. But, it's not possible to manually test #72 without porting Pathauto to 8.x first ;)

fago’s picture

Yep, this doesn't patch does not limit pathauto in any way.

jhodgdon’s picture

Thanks! Back to lurk mode. :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup
+++ b/core/modules/path/path.moduleundefined
@@ -283,19 +310,18 @@ function path_taxonomy_term_insert(Term $term) {
+      Drupal::service('path.crud')->save($source, $term->path->alias, $langcode, $pid);

Ick! :)

This was not introduced in this patch, but let's get this moved over to a Drupal::path() method in a follow-up.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.phpundefined
@@ -84,18 +84,18 @@ public function testEntityReferenceItem() {
-      'vid' => $this->term->vid,
+      'vid' => $this->term->bundle(),

This made me do a double-take, but "vid" here refers to "vocabulary ID" not "revision ID." Still, it's pretty hard to parse out that this is what bundle() is going to return. Berdir pointed me at #1233394: [Policy, no patch] Agree on a property naming pattern where we're discussing better DX around this. +100.

+++ b/core/modules/forum/forum.admin.incundefined
@@ -5,28 +5,32 @@
+ * @param \Drupal\taxonomy\Plugin\Core\Entity\Term $term
+ *   (optional) A forum or container term to be edited. Defaults to NULL.
...
+function forum_form_main($type, Term $term = NULL) {

@@ -34,37 +38,30 @@ function forum_form_main($type, $edit = array()) {
+ * @param \Drupal\taxonomy\Plugin\Core\Entity\Term $term
+ *   A forum term to be edited.
...
+function forum_form_forum($form, &$form_state, Term $term) {

It's always been beat into my face that we shouldn't type-hint with objects but rather with interfaces. Unfortunately, term interfaces don't exist yet, so this will have to be adjusted in #1391694: Use type-hinting for entity-parameters.

+++ b/core/modules/path/path.moduleundefined
@@ -262,18 +262,45 @@ function path_form_taxonomy_term_form_alter(&$form, $form_state) {
+/**
+ * Implements hook_entity_field_info().
+ */
+function path_entity_field_info($entity_type) {
+  if ($entity_type === 'taxonomy_term') {
+    $info['definitions']['path'] = array(
+      'type' => 'path_field',
+      'label' => t('The path alias'),
+      'computed' => TRUE,
+      'list' => TRUE,
+    );
+    return $info;
+  }

I really dislike that this hard-codes path module's hook_entity_field_info() implementation to taxonomy terms. I would expect that to be something like "foreach ($entity that exposes a user-facing path property...)"

Berdir pushed back and noted that currently this isn't technically introducing a regression, since path module hard-codes a hook_user and hook_taxonomy implementation, so this is just porting over already nasty code. I guess. :\ Still, let's make sure a "major" follow-up exists to get this cleaned up properly. And I don't think #1751210: Convert URL alias form element into a field and field widget is it (maybe with re-scoping), because that's turning URL aliases into a user-facing field, not an entity property.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -110,21 +110,39 @@ class Term extends Entity implements ContentEntityInterface {
+  protected function init() {
+    parent::init();
+    unset($this->tid);
+    unset($this->uuid);
+    unset($this->vid);
+    unset($this->name);
+    unset($this->weight);
+    unset($this->format);
+    unset($this->description);
+    unset($this->parent);

Yee-uck! Berdir says this is for IDE nice-ity and something we do in all other EntityNG entities. Something we should be able to clean up further with #1391694: Use type-hinting for entity-parameters as well.

Other than that, this all looks like good and fine stuff. I'm willing to commit this as-is, but we still need profiling, so needs work for that.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -needs profiling

Thanks, I was initially a bit worried when I saw the needs work but this doesn't look too bad ;)

What do you want to have profiled? I created a node with 24 terms (A-Z ;)) and I'm seeing a regression of 4-5%, both with xhprof and ab. taxonomy_get_tree() in some cases will be quite a bit worse than it already is but if we want to make terms properly translatable, we don't really have a choice (We need a loaded term that we can call label() on so that we get the right language) We need to find other ways to catch up, lazy-loaded terms might help or making that function a service that can be replaced with a cached/more performant solution when necessary.

There's not that much point in profiling every single NG conversion issue except to check if there are obvious problems with the actual conversion (which I can't see here). What we need to do is complete the conversion, throw out the BC mode, complete the few issues that we are working on that will improve EntityNG performance and then do some profiling and see what we can optimize. And make persistent entity load and render caches work :)

Wim Leers’s picture

#79: I think you addressed everything, besides a major follow-up for

+function path_entity_field_info($entity_type) {
+  if ($entity_type === 'taxonomy_term') {
Wim Leers’s picture

Issue summary: View changes

added remaining tasks

fago’s picture

There's not that much point in profiling every single NG conversion issue except to check if there are obvious problems with the actual conversion (which I can't see here). What we need to do is complete the conversion, throw out the BC mode, complete the few issues that we are working on that will improve EntityNG performance and then do some profiling and see what we can optimize. And make persistent entity load and render caches work :)

+1

klonos’s picture

We can do profiling but instead of have it block commit perhaps we should simply tag fixed issues with something like "performance regression" and "revisit" if there is serious regression. This way we can do a final profiling and try to optimize things that actually caused the regression (if this regression is still an issue once everything is in place).

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Ok checked with catch, he agrees with the assessment in #79. One moment.

webchick’s picture

Title: Convert taxonomy entities to the new Entity Field API » Change notice: Convert taxonomy entities to the new Entity Field API
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed and pushed to 8.x. Thanks! :D

Needs a change notice most likely, and let's get that file-up followed for path.module.

Wim Leers’s picture

Issue tags: -sprint

Huzzah! Awesome work, Berdir!

Berdir’s picture

Title: Change notice: Convert taxonomy entities to the new Entity Field API » Convert taxonomy entities to the new Entity Field API
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record, -Needs followup

Referenced this issue on http://drupal.org/node/1806650, added it there.

Created #1980822: Support any entity with path.module as requested. That will need a change notice once it's done, but not yet.

There are AFAIK no other API changes here, so I think we're good.

webchick’s picture

Rock, thanks Berdir! :)

andypost’s picture

Status: Fixed » Needs work

At least here needed follow-up to clean-up introduced inconsistency and continue in #1233394: [Policy, no patch] Agree on a property naming pattern

+++ b/core/modules/forum/forum.admin.incundefined
@@ -305,8 +295,8 @@ function _forum_parent_select($tid, $title, $child_type) {
-        $options[$term->tid] = str_repeat(' -- ', $term->depth) . $term->label();
...
+        $options[$term->id()] = str_repeat(' -- ', $term->depth) . $term->label();

+++ b/core/modules/forum/templates/forum-list.tpl.phpundefined
@@ -55,8 +55,8 @@
         <?php print str_repeat('</div>', $forum->depth); ?>

+++ b/core/modules/options/options.api.phpundefined
@@ -61,7 +61,7 @@ function hook_options_list($field, $instance, $entity) {
-        $options[$term->tid] = str_repeat('-', $term->depth) . $term->label();
+        $options[$term->id()] = str_repeat('-', $term->depth) . $term->label();

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -110,21 +110,39 @@ class Term extends Entity implements ContentEntityInterface {
+    unset($this->tid);
+    unset($this->uuid);
+    unset($this->vid);
+    unset($this->name);
+    unset($this->weight);
+    unset($this->format);
+    unset($this->description);
+    unset($this->parent);

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/entity_reference/selection/TermSelection.phpundefined
@@ -67,7 +67,7 @@ public function getReferencableEntities($match = NULL, $match_operator = 'CONTAI
-            $options[$vocabulary->id()][$term->tid] = str_repeat('-', $term->depth) . check_plain($term->name);
+            $options[$vocabulary->id()][$term->id()] = str_repeat('-', $term->depth) . check_plain($term->name);

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -109,4 +111,60 @@ public function resetCache(array $ids = NULL) {
+  public function baseFieldDefinitions() {
+    $properties['tid'] = array(

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -240,25 +240,18 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
-      '#prefix' => isset($term->depth) && $term->depth > 0 ? theme('indentation', array('size' => $term->depth)) : '',
+      '#prefix' => isset($term->depth->value) && $term->depth->value > 0 ? theme('indentation', array('size' => $term->depth->value)) : '',

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -783,26 +783,27 @@ function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities
         $term->depth = $depth;
         unset($term->parent);

@@ -1098,7 +1099,7 @@ function taxonomy_allowed_values($field, $instance, EntityInterface $entity) {
-          $options[$term->tid] = str_repeat('-', $term->depth) . $term->label();
+          $options[$term->id()] = str_repeat('-', $term->depth) . $term->label();

there's no depth definition at all? so why this converted?

tid => id()
vid => vid->value || bundle() ?????
name => name->value || label() ??????

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

@@ -84,18 +84,18 @@ public function testEntityReferenceItem() {
-    $this->assertEqual($term->name, $new_name);
+    $this->assertEqual($term->name->value, $new_name);
...
-      'vid' => $this->term->vid,
+      'vid' => $this->term->bundle(),

+++ b/core/modules/forum/forum.admin.incundefined
@@ -305,8 +295,8 @@ function _forum_parent_select($tid, $title, $child_type) {
-        $options[$term->tid] = str_repeat(' -- ', $term->depth) . $term->label();
+      if (!in_array($term->id(), $exclude)) {
+        $options[$term->id()] = str_repeat(' -- ', $term->depth) . $term->label();

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryRelationshipTest.phpundefined
@@ -147,18 +147,18 @@ public function testQuery() {
-      ->condition("$this->fieldName.entity.name", $this->terms[0]->name, '<>')
+      ->condition("$this->fieldName.entity.name", $this->terms[0]->name->value, '<>')

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.phpundefined
@@ -352,7 +352,7 @@ function testBreadCrumbs() {
-      $this->assertBreadcrumb($link['link_path'], $trail, $term->name, $tree);
+      $this->assertBreadcrumb($link['link_path'], $trail, $term->label(), $tree);

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/entity_reference/selection/TermSelection.phpundefined
@@ -67,7 +67,7 @@ public function getReferencableEntities($match = NULL, $match_operator = 'CONTAI
-            $options[$vocabulary->id()][$term->tid] = str_repeat('-', $term->depth) . check_plain($term->name);
+            $options[$vocabulary->id()][$term->id()] = str_repeat('-', $term->depth) . check_plain($term->name);

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument/IndexTidDepth.phpundefined
@@ -149,7 +149,7 @@ public function query($group_by = FALSE) {
-      return check_plain($term->name);
+      return check_plain($term->label());

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument/Taxonomy.phpundefined
@@ -27,7 +27,7 @@ function title() {
-        return check_plain($term->name);
+        return check_plain($term->label());

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.phpundefined
@@ -21,14 +21,14 @@ class TermFormController extends EntityFormController {
-      '#default_value' => $term->name,
+      '#default_value' => $term->name->value,

inconsistency

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/field/TaxonomyIndexTid.phpundefined
@@ -119,8 +119,8 @@ function pre_render(&$values) {
-        $this->items[$term->node_nid][$term->tid]['vocabulary_vid'] = $term->bundle();
-        $this->items[$term->node_nid][$term->tid]['vocabulary'] = check_plain($vocabularies[$term->bundle()]->label());
+        $this->items[$term->node_nid][$term->tid]['vocabulary_vid'] = $term->vid;
+        $this->items[$term->node_nid][$term->tid]['vocabulary'] = check_plain($vocabularies[$term->vid]->label());

but here bundle changed back to vid without ->value

andypost’s picture

Status: Needs work » Needs review
FileSize
8.46 KB

There's primary trouble in taxonomy_get_tree() which could return objects and entities depending on $load_entities argument so it's not clear how to use $term->id() once object is returned from db_select()

Also it shows that taxonomy badly covered with tests

Patch tries to fix depth, but also 'depth' needs definition in storage controller

andypost’s picture

Status: Needs review » Needs work

The last submitted patch, 1818560-termng-89.patch, failed testing.

Berdir’s picture

Can we do that in a follow-up issue that cleans up depth/parent/parents? depth could actually be different per parent, so it might make sense to make that a property of $term->parent->depth (and $term->parent[1]->depth), which in turn would be a computed field.

It's not as inconsistent as you make it.

vid()/id()/label() should be used when possible, in case of terms, there's the mess with taxonomy_get_tree() which this issue did *not* introduce, just used it in more places.

In e.g. an edit form for example, you don't want to edit the (possibly altered) label of a term, you want to edit the *name*, so, $term->name->value, not $term->label().

The last example there is because that class does a manual query to load terms, it was broken in HEAD, has no test coverage and this issue fixed it. Yes, it should use entity load/EFQ but that's outside the context of this issue.

andypost’s picture

Status: Needs work » Fixed
andypost’s picture

Issue summary: View changes

update remaining tasks

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.