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
Comment | File | Size | Author |
---|---|---|---|
#51 | taxo-trees-1976298.50.patch | 25.9 KB | larowlan |
#51 | interdiff.txt | 2.15 KB | larowlan |
#47 | taxo-trees-1976298.47.patch | 25.11 KB | larowlan |
Comments
Comment #1
larowlanComment #2
larowlanFirst pass
Comment #4
larowlanI knew there would be somewhere the statics needed clearing.
Comment #6
larowlanMissed one.
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedGood job!
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?
Comment #8
larowlanAfter 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.
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedx-posting #1742850: Follow-up for entity_load_multiple_by_properties()
i think this will show the way pretty much
Comment #10
xjmSo, 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 onVocabularyInterface
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.
Comment #11
xjmOopsie, xpost.
#1391694: Use type-hinting for entity-parameters is the issue I'm alluding to in #10.
Comment #12
xjmMaybe this one too.
Comment #13
andypostRelated #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
Comment #14
larowlanComment #15
andypostRelated #1988712: Move taxonomy_check_vocabulary_hierarchy() into storage controller
Comment #16
tim.plunkettThis needs discussion again.
Comment #17
jibran#6: taxonomy-service-1976298.6.patch queued for re-testing.
Comment #19
xjmThanks 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.
Comment #20
Crell CreditAttribution: Crell commentedThis doesn't need a WSCCI tag.
Comment #21
pfrenssenIf 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?
Comment #22
larowlanbumping Forum module has OO wrappers around these so it can be unit tested.
Comment #23
larowlanNew patch, new issue title, new issue summary, new issue scope.
No interdiff, clean start
Comment #24
larowlanComment #25
larowlanComment #26
jibranDo you think adding unit tests for this is a good idea?
more then 80 chars
Comment #28
larowlanentity_manager != entity.manager
fixes #26
Unit tests are difficult because all the methods include database queries.
Comment #30
larowlanMore fixes
Comment #34
larowlanre-roll
Comment #36
larowlanComment #38
larowlanComment #39
jibranOver all huge +1 just a minor issue.
I think this is not right. It is same in the old code.
Comment #40
larowlanFrom the code
I guess that means it's on purpose?
Comment #41
larowlanDraft change notice https://www.drupal.org/node/2328205
Reroll
Comment #42
jibranOk thank you for explaining it.
Comment #43
larowlanRe-roll, new tags, new title
Comment #44
BerdirI'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.
Would loadAllParents() make more sense as a method name?
Comment #45
larowlanFixes 44.2
Spoke with @Berdir about 44.1 on irc,
Comment #47
larowlanre-roll
Comment #48
larowlanback to rtbc - re-roll only
Comment #49
jibran+1 for RTBC
Comment #50
alexpottThe swapping of the argument order here unfortunate given that one extends the other.
Whilst we're messing with the constructor can we fix the spelling of interface.
$var['trees'] should be $vars['trees']
The swapping of the order of the arguments seems really awkward here consider that one extends the other.
Comment #51
larowlanFixes #50
Comment #52
jibranThank you back to RTBC.
Comment #53
alexpottCommitted 2955942 and pushed to 8.0.x. Thanks!