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

Files: 
CommentFileSizeAuthor
#30 taxo-trees-1976298.30.patch26.34 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,316 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#30 interdiff.txt5.28 KBlarowlan
#28 taxo-trees-1976298.28.patch21.63 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,332 pass(es), 24 fail(s), and 13 exception(s).
[ View ]
#28 interdiff.txt1.59 KBlarowlan
#2 taxonomy-service-1976298.1.patch12.69 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 54,950 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Comments

Assigned:Unassigned» larowlan

Status:Active» Needs review
StatusFileSize
new12.69 KB
FAILED: [[SimpleTest]]: [MySQL] 54,950 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

First pass

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new13.52 KB
FAILED: [[SimpleTest]]: [MySQL] 54,939 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new858 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.

Status:Needs work» Needs review
StatusFileSize
new14.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-service-1976298.6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new904 bytes

Missed one.

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?

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.

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

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.

Status:Needs review» Postponed

Oopsie, xpost.

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

Maybe this one too.

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

Assigned:larowlan» Unassigned

Status:Postponed» Active

This needs discussion again.

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

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.

Issue tags:-WSCCI

This doesn't need a WSCCI tag.

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?

Issue summary:View changes

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

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
StatusFileSize
new21.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,310 pass(es), 82 fail(s), and 15 exception(s).
[ View ]

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

Status:Needs work» Needs review

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.

Status:Needs work» Needs review
StatusFileSize
new1.59 KB
new21.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,332 pass(es), 24 fail(s), and 13 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new5.28 KB
new26.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,316 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

More fixes

Status:Needs review» Needs work

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