Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Taxonomy.module
contains deprecated functions that should be removed before 8.0 release
Part of #2205673: [META] Remove all @deprecated functions marked "remove before 8.0"
Proposed resolution
Each function usage should be replaced with appropriate method from taxonomy storage controller, that should be taken from entity manager (\Drupal::entityManager()->getStorage('taxonomy_term')
) or from injected entity manager into forms and controllers ($this->entityManager->getStorage('taxonomy_term')
)
function | replacement |
---|---|
taxonomy_term_load_parents()
| \Drupal\taxonomy\TermStorageInterface::loadParents()
|
taxonomy_term_load_children()
| \Drupal\taxonomy\TermStorageInterface::loadChildren()
|
taxonomy_get_tree()
| \Drupal\taxonomy\TermStorageInterface::loadTree() |
Remaining tasks
fix patch, rtbc, commit
User interface changes
no
API changes
no
Comment | File | Size | Author |
---|---|---|---|
#34 | remove_usage_of-2452577-34.patch | 19.97 KB | JeroenT |
#34 | interdiff.txt | 970 bytes | JeroenT |
#33 | remove_usage_of-2452577-33.patch | 20.01 KB | JeroenT |
#26 | interdiff-24-26.txt | 3.52 KB | tadityar |
#26 | remove_usage_of-2452577-26.patch | 19.99 KB | tadityar |
Comments
Comment #1
arpitr CreditAttribution: arpitr commentedComment #2
arpitr CreditAttribution: arpitr commentedAdding patch to replace deprecated function calls.
Comment #4
a_thakur CreditAttribution: a_thakur commentedComment #5
dpopdan CreditAttribution: dpopdan commentedComment #6
lhangea CreditAttribution: lhangea commentedUpdating to needs review so that the patch is tested
Comment #8
JeroenTCreated a new patch.
Comment #9
JeroenTCoupled CR and created follow-up issue to remove the functions: #2465463: Remove deprecated taxonomy_* functions..
Comment #11
JeroenT.
Comment #12
Mile23Comment #13
Mile23Some stuff to get started:
This method has enough occurrences of
getStorage()
that you might just call it once and reference the manager. Like:Needs leading \ before the second class name.
Consolidate the calls to
getManager()
like in #1 above.Test classes descending from
TestBase
have their own dependency-injected container for the mocked Drupal under test.So instead of saying
\Drupal::entityManager()
you should say$this->container->get('entity.manager')
For both of these, and for the other call to
getStorage()
down below it, grab a reference to the storage manager at the top of the function, outside theforeach()
loop, and then just call the methods on that inside the loop.Thanks!
Comment #14
Mile23Note: this is kind of a duplicate of this one: #2405365: Remove usage of taxonomy_term_load_parents()
Comment #15
AjitSworking on it
Comment #16
AjitSTrying
Comment #18
tadityar CreditAttribution: tadityar commentedTrying.
Comment #20
JeroenTHi tadityar,
Thank you for your patch.
I think the patch of AjitS (#16) is almost there, but there are still some failing tests in TermTest.php.
I haven't test it, but i think it's because the loadParents(), loadChildren() and loadTree() methods are directly called on the entitymanager instead of on the taxonomy storage.
So
should be changed to :
Comment #21
andypostupdated IS
Comment #22
AjitS@JeroenT You are right. That was the fatal error!
Correcting the error for now. Will collect the taxonomy storage in a single variable in the next patch.
Comment #23
AjitSBack to needs work as per #13.
Will update the patch soon.
Comment #24
AjitSMoved the taxonomy storage to variables wherever possible. IMO, the priority of the issue is not minor, as getting the storage at 10 different places, unnecessarily, might have performance issues.
Comment #25
andypost2 more places to fix + docs error
I'd suggest to use interface here
\Drupal\taxonomy\TermStorageInterface
that defines this methodsnot controller that could be swapped and there's no such thing *TSController*
Better to get the storage to separate variable here too
the same
Comment #26
tadityar CreditAttribution: tadityar commentedIs this correct?
Comment #27
andypostyep, thanx
Comment #28
xjmGreat work @tadityar -- this is looking good! Two points of feedback:
Any particular reason we're getting the entity manager from the available
$this->container
in some simpletests, but using the static method wrapper for the service in others? Not a big deal either way.In code comments, we should reference the default implementation of the service (class and method name), like the first hunk I've highlighted here does. So let's change the second hunk to do so as well.
Closed #2405365: Remove usage of taxonomy_term_load_parents() as a duplicate of this issue per #14. Thanks @Mile23.
Comment #29
Mile23Speaking to #28.1, there is this issue which should provide some clarity, but doesn't: #2087751: [policy, no patch] Stop using $this->container in web tests
Comment #30
xjmAha, that's what I was thinking of. Thanks @Mile23.
Comment #31
tadityar CreditAttribution: tadityar commentedThank you for the reviews :)
Should this issue get postponed until something has been decided in #2087751: [policy, no patch] Stop using $this->container in web tests then? Or should I just revise the comments and leave the code as is?
Comment #32
xjm@tadityar, I think it's fine to leave them as you have in the patch, even though it's not quite consistent. If the policy is decided one way or another, there will be a whole lot to clean up. So I think it's just the second point, to reference the class in the docs. Thanks!
Comment #33
JeroenTCreated straight reroll.
Patch attached.
Comment #34
JeroenTAddressed #28.2 as suggested by xjm. Patch attached.
Comment #35
andypostLooks good to me
Comment #36
Mile23My IDE couldn't find usages of taxonomy_term_load_parents(), taxonomy_term_load_children(), or taxonomy_get_tree() after applying the patch.
Comment #37
xjmThanks all! I confirmed that all three of these functions are deprecated for removal in 8.0.0 and that #2505851: Remove deprecated function taxonomy_* already has an attached change record for them. I also confirmed that no usages or references remain:
This issue is a prioritized change as per https://www.drupal.org/core/beta-changes and its benefits outweigh any disruption. Committed and pushed to 8.0.x.