Updated: Comment #N

Problem/Motivation

Forum manager, OverviewTerms and various other places still have to call the procedural taxonomy_term_get_parents(), taxonomy_term_get_parents_all(), taxonomy_get_tree() functions because we have no OO equivalent.

Proposed resolution

Move these to methods on the TermStorageController

Remaining tasks

Make it green
Review

User interface changes

None

API changes

taxonomy_term_load_parents(), taxonomy_term_load_parents_all(), taxonomy_term_load_children(), taxonomy_get_tree() all marked as deprecated.
TermStorageControllerInterface::loadParents() and TermStorageControllerInterface::loadChildren() now return Term objects instead of array of tids
TermStorageControllerInterface::loadTree() now returns an array of all term objects in the tree. Each term object is extended to have "depth" and "parents" attributes in addition to its normal ones (aka the original return of taxonomy_get_tree()).
New TermStorageControllerInterface::loadParentsAll().
Note that the only calls to the changes methods on TermStorageControllerInterface were inside the deprecated functions.

Original report by @larowlan

Follow up for #1971384: [META] Convert page callbacks to controllers

taxonomy_get_tree() needs to be a service that can be injected into \Drupal\taxonomy\Form\OverviewTerms and other objects.
Procedural wrappers should be retained for time being but marked as deprecated.

Related issues

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Assigned: Unassigned » larowlan
larowlan’s picture

Status: Active » Needs review
FileSize
12.69 KB

First pass

Status: Needs review » Needs work

The last submitted patch, taxonomy-service-1976298.1.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
13.52 KB
858 bytes

I knew there would be somewhere the statics needed clearing.

Status: Needs review » Needs work

The last submitted patch, taxonomy-service-1976298.3.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
14.11 KB
904 bytes

Missed one.

ParisLiakos’s picture

Good job!

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Taxonomy.phpundefined
@@ -0,0 +1,223 @@
+  public function clearCache() {

I think we should name it reset() to go inline with rest of core

Besides that why not have this logic to the vocabulary storage controller and register a new service?

larowlan’s picture

Status: Needs review » Postponed

After discussing with @xjm, @ParisLiakos and other in irc - postponing until we decide best approach for this, whether that be on Vocab entity, Vocab Storage Controller, or as a service.

ParisLiakos’s picture

x-posting #1742850: Follow-up for entity_load_multiple_by_properties()
i think this will show the way pretty much

xjm’s picture

Status: Postponed » Needs review
Issue tags: +DX (Developer Experience), +Increases learning curve

So, I love the idea of adding a getTree() method to vocabularies, and I think it would make sense for such a method to additionally be on VocabularyInterface when such comes to exist. I'm less excited about the idea of making taxonomy a service, since that will drastically increase the percentage of developers who have to grok the container before they can contribute D8 modules.

Adding a couple hopefully complementary tags. Let's consider this carefully.

xjm’s picture

Status: Needs review » Postponed

Oopsie, xpost.

#1391694: Use type-hinting for entity-parameters is the issue I'm alluding to in #10.

xjm’s picture

Maybe this one too.

andypost’s picture

Related #1980982: Clean-up procedural wrappers for taxonomy against save|delete
Also needs re-roll after #1818560: Convert taxonomy entities to the new Entity Field API because $load_entities is FALSE by default so objects from function not a Term class

larowlan’s picture

Assigned: larowlan » Unassigned
andypost’s picture

tim.plunkett’s picture

Status: Postponed » Active

This needs discussion again.

jibran’s picture

The last submitted patch, taxonomy-service-1976298.6.patch, failed testing.

xjm’s picture

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Crell’s picture

Issue tags: -WSCCI

This doesn't need a WSCCI tag.

pfrenssen’s picture

If we keep the procedural wrappers, does this really increase the learning curve? And, isn't it to be expected that module developers will learn to work with the dependency injection container within their first week of developing for D8?

larowlan’s picture

Issue summary: View changes

bumping Forum module has OO wrappers around these so it can be unit tested.

larowlan’s picture

Title: Convert taxonomy_get_tree() and associated functions into a Taxonomy service, retain procedural wrappers. » Move taxonomy_get_tree() and associated functions to Taxonomy storage, retain procedural wrappers.
Issue summary: View changes
FileSize
21.63 KB

New patch, new issue title, new issue summary, new issue scope.
No interdiff, clean start

larowlan’s picture

Status: Needs work » Needs review
larowlan’s picture

jibran’s picture

Issue tags: +API clean-up

Do you think adding unit tests for this is a good idea?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.php
@@ -86,47 +131,162 @@ public function updateTermHierarchy(EntityInterface $term) {
+              // Reset pointers for child lists because we step in there more often

more then 80 chars

Status: Needs review » Needs work

The last submitted patch, 23: taxo-trees-1976298.23.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
21.63 KB

entity_manager != entity.manager
fixes #26
Unit tests are difficult because all the methods include database queries.

Status: Needs review » Needs work

The last submitted patch, 28: taxo-trees-1976298.28.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.28 KB
26.34 KB

More fixes

Status: Needs review » Needs work

The last submitted patch, 30: taxo-trees-1976298.30.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 30: taxo-trees-1976298.30.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
24.69 KB

re-roll

Status: Needs review » Needs work

The last submitted patch, 34: taxo-trees-1976298.34.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
24.64 KB

Status: Needs review » Needs work

The last submitted patch, 36: taxo-trees-1976298.36.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.94 KB
25.12 KB
jibran’s picture

Over all huge +1 just a minor issue.

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -86,50 +136,165 @@ public function updateTermHierarchy(EntityInterface $term) {
+              $term = clone $term;

I think this is not right. It is same in the old code.

larowlan’s picture

From the code


// Clone the term so that the depth attribute remains correct
// in the event of multiple parents.

I guess that means it's on purpose?

larowlan’s picture

FileSize
25.12 KB

Draft change notice https://www.drupal.org/node/2328205
Reroll

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Ok thank you for explaining it.

larowlan’s picture

Title: Move taxonomy_get_tree() and associated functions to Taxonomy storage, retain procedural wrappers. » Move taxonomy_get_tree() and associated functions to Taxonomy storage, deprecate procedural wrappers.
Issue tags: -Needs architectural review, -Increases learning curve, -FormInterface, -WSCCI-conversion +Entity Field API
FileSize
25.12 KB

Re-roll, new tags, new title

Berdir’s picture

  1. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -86,50 +136,165 @@ public function updateTermHierarchy(EntityInterface $term) {
       public function loadParents($tid) {
    -    $query = $this->database->select('taxonomy_term_field_data', 't');
    -    $query->join('taxonomy_term_hierarchy', 'h', 'h.parent = t.tid');
    

    I'm wondering if should just make parents a normal field that we load when loading the entity. We're trying to make it an entity reference (so that serialization works) and it's really a pain.

    This was a performance optimization to not do it and also to not update it on save unless we have to, but that's kind of a joke now given that we have a data table and stuff. And for loading, we have the entity storage cache now.

  2. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -86,50 +136,165 @@ public function updateTermHierarchy(EntityInterface $term) {
    +  public function loadParentsAll($tid) {
    

    Would loadAllParents() make more sense as a method name?

larowlan’s picture

FileSize
2.31 KB
25.12 KB

Fixes 44.2
Spoke with @Berdir about 44.1 on irc,

larowlan
berdir: yeah, agreed - but are you happy to press ahead on this cleanup and revisit when we have an actual ER field?

berdir
larowlan: yes, ok with that

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: taxo-trees-1976298.45.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
25.11 KB

re-roll

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc - re-roll only

jibran’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/forum/src/Form/Overview.php
    @@ -37,7 +37,7 @@ class Overview extends OverviewTerms {
       public function __construct(EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler) {
    -    parent::__construct($module_handler);
    +    parent::__construct($module_handler, $entity_manager);
    
    +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -26,13 +27,23 @@ class OverviewTerms extends FormBase {
    -  public function __construct(ModuleHandlerInterface $module_handler) {
    +  public function __construct(ModuleHandlerInterface $module_handler, EntityManagerInterface $entity_manager) {
         $this->moduleHandler = $module_handler;
    +    $this->storageController = $entity_manager->getStorage('taxonomy_term');
    

    The swapping of the argument order here unfortunate given that one extends the other.

  2. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -26,13 +27,23 @@ class OverviewTerms extends FormBase {
        * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    

    Whilst we're messing with the constructor can we fix the spelling of interface.

  3. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -189,4 +354,29 @@ public function getNodeTerms($nids, $vocabs = array(), $langcode = NULL) {
    +    unset($vars['parents'], $vars['parentsAll'], $vars['children'], $vars['treeChildren'], $vars['treeParents'], $vars['treeTerms'], $var['trees']);
    

    $var['trees'] should be $vars['trees']

  4. +++ b/core/modules/forum/src/Form/Overview.php
    @@ -37,7 +37,7 @@ class Overview extends OverviewTerms {
       public function __construct(EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler) {
    -    parent::__construct($module_handler);
    +    parent::__construct($module_handler, $entity_manager);
    
    +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -26,13 +27,23 @@ class OverviewTerms extends FormBase {
    +  /**
        * Constructs an OverviewTerms object.
        *
        * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
        *   The module handler service.
    +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
    +   *   The entity manager service.
        */
    -  public function __construct(ModuleHandlerInterface $module_handler) {
    +  public function __construct(ModuleHandlerInterface $module_handler, EntityManagerInterface $entity_manager) {
    

    The swapping of the order of the arguments seems really awkward here consider that one extends the other.

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.15 KB
25.9 KB

Fixes #50

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thank you back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2955942 and pushed to 8.0.x. Thanks!

  • alexpott committed 2955942 on 8.0.x
    Issue #1976298 by larowlan: Move taxonomy_get_tree() and associated...

Status: Fixed » Closed (fixed)

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