Properly deprecate TermStorage::deleteTermHierarchy() and ::updateTermHierarchy()

Imo itvwas better to reopen #2785693: Follow up #2543726: @todo deleteTermHierarchy and updateTermHierarchy as there's todo pointing there, just need to check if there other todo refs to the issue

Discussed with xjm, since this wasn't actually marked as deprecated via either @deprecated or trigger_error(), and there are still usages (even if no-op), it seems better to deprecate in 9.x for removal in 10.x at this point. Also these are just cruft rather than having a specific maintenance cost.

So... moving to 9.1.x, but it would be good to have an RTBC patch ready to go so it doesn't get forgotten again. I think everything is ready to go here except the version numbers.

Fixed it, meantime https://www.drupal.org/node/2936675 references 8.6.0

To fix all of them at the root, rather than adding more custom code and work-arounds, we changed Term to not use custom field storage for its parent field. This also allowed custom code to be deleted. It strengthens Drupal 8's Entity API: more things work in a predictable way.

There are two/one calls to it left in searchable contrib: http://grep.xnddx.ru/search?text=deleteTermHierarchy / http://grep.xnddx.ru/search?text=updateTermHierarchy.

Feels a bit strange to deprecate them when the calls to them are actually broken already, usually we do deprecation tests to make sure that deprecated API's still work, but there's nothing left here that could possibly work.

IMHO a clear error is preferable to things silently not working :)

Aren't they a no-op rather than broken though? The thing you want to do happens, just not because you called the methods.

If they actually would cause bugs in contrib modules calling the methods, then agree it's better to get rid of them but that didn't seem to be the case.

They used to save term parent information. The only reason to call them directly is to _only_ save that information, without saving the term too.

If you look at http://grep.xnddx.ru/node/29580503#line-151 for example, or http://grep.xnddx.ru/node/29410221#line-391, the only two usages that were found, then they don't save the term, which IMHO means that the logic there is now broken?

So if you look at Term::postDelete():

https://api.drupal.org/api/drupal/core%21modules%21taxonomy%21src%21Enti...

When a term is deleted, it loads all the children of that term, updates the parent information, then resaves them.

This is what http://grep.xnddx.ru/node/29580503#line-151 is trying to do manually. So for that example I think it's just redundant/no-op code but still working.

acquia_commercemanager (http://grep.xnddx.ru/node/29410221#line-391) is also saving the parent term for the children that it's trying to break relationships for, but I can't really tell what the intention actually is there from a quick look.

Back to 8.x while we figure this out properly.

a clear error is preferable

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -93,12 +93,16 @@ public function resetCache(array $ids = NULL) {
+    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Parent references are automatically cleared when deleting a taxonomy term. See https://www.drupal.org/node/2936675', E_USER_DEPRECATED);
...
+    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Parent references are automatically updated when updating a taxonomy term. See https://www.drupal.org/node/2936675', E_USER_DEPRECATED);

Probably message just needs to be appended with "no-op since 8.6.0"

At this point, we'll need to deprecate this in 9.1.x instead (for removal in D10).

It's ready

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -93,12 +93,16 @@ public function resetCache(array $ids = NULL) {
+    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. It is a no-op since 8.6.0. Parent references are automatically cleared when deleting a taxonomy term. See https://www.drupal.org/node/2936675', E_USER_DEPRECATED);
...
+    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. It is a no-op since 8.6.0. Parent references are automatically updated when updating a taxonomy term. See https://www.drupal.org/node/2936675', E_USER_DEPRECATED);

Inteteresting/efficient choice to use __METHOD__. I haven't seen this but we do it in 52 places in 8.8 (out of 901 @trigger_error() (many of which are not methods obviously; I didn't dig deeper than that). I wondered if it might fail a phpcs rule that did not take into account this possible format, but it looks like there isn't one enabled in the core ruleset (at least yet). The patch passes. So that's fine.

However, we still haven't addressed #8 - #11. We should at least file issues in those two project's queues, but that doesn't address custom code that might also be calling this. Unsilencing a deprecation is disruptive so I'm loathe to do that at almost-RC for something that's a no-op, but it needs discussion.

I debated whether to separate the proper deprecation from the error so these don't sit around until Drupal 11. I'll ask @catch what he thinks also.

OK, discussed with @catch. Let's get three followups filed (two for the contrib projects, one for core to figure out how to address the lack-of-error generally) and then commit this patch as-is to 9.1.x so at least we have a clean deprecation as soon as possible.

For the core followup, it looks like I raised concerns along the same line as @Berdir's and my feedback was not taken into account (Wim kind of missed the point). Everyone on that issue was focused on the REST bugfix and ignored the implications for Taxonomy itself. Things I said then include:

Regarding #321: Having fixes for known contrib modules is, unfortunately, not the only thing we need to consider in terms of disruption. If we know about multiple contrib modules that will be broken by this, then there are very likely sites with custom code that also relies on the current behavior that will be broken.

Another thing that we discussed is that we'd like to explore some way of warning developers that the table is deprecated in 8.5.x. I would have preferred that that happen before commit, but can we get a followup issue for that?

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -93,12 +93,16 @@ public function resetCache(array $ids = NULL) {
+    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. It is a no-op since 8.6.0. Parent references are automatically cleared when deleting a taxonomy term. See https://www.drupal.org/node/2936675', E_USER_DEPRECATED);
...
+    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. It is a no-op since 8.6.0. Parent references are automatically updated when updating a taxonomy term. See https://www.drupal.org/node/2936675', E_USER_DEPRECATED);

We need to update the deprecation message to be 10.1.0 for 11 now.

I think we can split the difference between an error and a silenced deprecation by unsilencing the deprecation.

Rerolled patch #14 for 10.1.x.

Unsure how to deal with this:

I think we can split the difference between an error and a silenced deprecation by unsilencing the deprecation.

, so I blatantly ignored it for now.

Thank you, is it backportable to 10.0.1?

Thank you, is it backportable to 10.0.1?

I'm a bit unsure if we are allowed to introduce new deprecations in 10.0.1.
But the plain diff of the MR applies cleanly to the 10.0.x branch, so technically I think we could.

Let's await the verdict of the core committers :)

I think we would have been better off committing the silenced deprecation three years ago than trying to figure out how to do an error and not getting the issue in.

Have gone ahead and committed it with @trigger_error(), we can change that to trigger_error() (not @) in a follow-up if we want to, but also deprecation testing and phpstan have moved on a lot since this was last discussed.

Committed/pushed to 10.1.x, thanks!

Don't think we can/should backport this to 10.0.x since it'll potentially introduce new deprecation failures for modules that have just been ported to 10.0

@catch, thanks for clearing up the silenced (@) mystery, I went looking all over the place for a deprecation that was code-wise suppressed or something similar.

Reading is hard already, understanding what you read turns out to be even harder...

This is tagged for followups, see #16 - #18.
My searches found usages of updateTermHierarchy and/or deleteTermHierarchy in the following projects:

• entity_taxonomy - abandoned, updated 8 May 2022
• rdf_entity
• taxonomy_manager
• acquia_commercemanager

Noting them here so anyone can make the issues. I am not able to do it now.

TBH I believe that warning contrib about core changes was maybe something we did in 2020, when #16 -18 were posted, and not so much what we do nowadays?

Anyway:

• entity_taxonomy - abandoned, updated 8 May 2022
• rdf_entity - #3322643: Remove no-op lines in TermRdfStorage
• taxonomy_manager - #3322642: Remove no-op lines in TaxonomyManagerHelper
• acquia_commercemanager - Seems abandoned, no issue queue to add an issue to.

@Spokje, Thank you for making the follow ups, despite your reservations.