Problem/Motivation

There is "legacy code" in three Views handlers that was removed from core in #1552396: Convert vocabularies into configuration but immediately and accidentally reintroduced in #1850792-26: Make init() method consistent across all views plugins. Because of the isset() check, it has been dead code this entire time, and the whole init() method can be removed in both cases.

The init() code was to handle the conversion of legacy Vocabulary IDs to the newer Vocabulary machine names.
While that init() patch was being worked on, the larger conversion was committed.
Due to a bad rebase, the legacy workaround was reintroduced.

Steps to reproduce

N/A

Proposed resolution

Remove the init() code

Remaining tasks

Comment #3 raises questions about backwards compatibility and how it should be addressed. This needs to be discussed for the issue to move on.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#13 3221149-taxoview-13.patch5.56 KBtim.plunkett
#7 3221149-7.patch6.96 KBguilhermevp
#2 3221149-taxoview-2.patch5.66 KBtim.plunkett

Issue fork drupal-3221149

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3221149-taxoview-2.patch5.66 KB

This would be broken when #3039039: Deprecate some procedural functions in taxonomy.module lands, so I wrote the patch assuming that lands. Not queueing for testing until that lands.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Active » Needs work 
+++ b/core/modules/taxonomy/src/Plugin/views/argument_default/Tid.php
@@ -72,24 +70,6 @@ public static function create(ContainerInterface $container, array $configuratio
diff --git a/core/modules/taxonomy/src/Plugin/views/argument_validator/Term.php b/core/modules/taxonomy/src/Plugin/views/argument_validator/Term.php

diff --git a/core/modules/taxonomy/src/Plugin/views/argument_validator/Term.php b/core/modules/taxonomy/src/Plugin/views/argument_validator/Term.php
deleted file mode 100644

Is there any BC concerns with just removing this class? What if code is extending it? Should we deprecate it?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Closing #3220647: Remove "legacy vids" handling from taxonomy Views plugins as duplicate.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: +Needs reroll
daffie’s picture

daffie commented

+1 for comment #3 by @tedbow

guilhermevp’s picture

guilhermevp commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3221149-7.patch6.96 KB

Re-rolled patch. Moved to needs review for testing but comment #3 needs to be addressed.

Status: Needs review » Needs work

The last submitted patch, 7: 3221149-7.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Postponed

@guilhermevp The patch in #2 is purposefully rolled to assume that #3039039: Deprecate some procedural functions in taxonomy.module lands first. It did not need automatic testing or a reroll yet.

I do not think core/modules/taxonomy/src/Plugin/views/argument_validator/Term.php needs a deprecation. Views handlers are plugins which are internal and not part of the API.

Postponing to prevent any unnecessary work for now.

andypost’s picture

andypost
he/him
Primary language Russian
commented

The #3039039-39: Deprecate some procedural functions in taxonomy.module convinces to unpostpone it

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ /dev/null
@@ -1,87 +0,0 @@
-class TermName extends Entity {

I think it needs deprecation and update hook with test

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Postponed » Needs work

Un-postponing as per #3039039-39: Deprecate some procedural functions in taxonomy.module.

+++ b/core/modules/taxonomy/src/Plugin/views/argument_default/Tid.php
index 1a81b2f952..0000000000
--- a/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php

--- a/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
+++ /dev/null

No sure this is correct. Why is this argument validator dropped?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new 3221149-taxoview-13.patch5.56 KB

I don't know why either. That's only in @guilhermevp's patch, not mine. Rerolling mine for 9.3.x

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Needs work 
+++ b/core/modules/taxonomy/taxonomy.views.inc
@@ -80,6 +80,5 @@ function taxonomy_field_views_data_alter(array &$data, FieldStorageConfigInterfa
-  $plugins['entity:taxonomy_term']['class'] = 'Drupal\taxonomy\Plugin\views\argument_validator\Term';

Doesn't need an update path because all the config is pointing to entity:taxonomy_term.

Probably does need an empty post_update hook just to clear caches, though.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think we need to deprecate \Drupal\taxonomy\Plugin\views\argument_validator\Term because it is being used in contrib and this change as is will break some modules - see http://grep.xnddx.ru/search?text=Drupal%5Ctaxonomy%5CPlugin%5Cviews%5Car...

I think we remove \Drupal\taxonomy\Plugin\views\argument_validator\Term::init() and add a constructor with an @trigger_error - as per usual plugin deprecation.

Also I think we can remove taxonomy_views_plugins_argument_validator_alter() entirely. The title change doesn't seem necessary and the provider is now incorrect.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: Unassigned » claudiu.cristea

Assigning to work on it.

claudiu.cristea opened merge request !889

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

Implemented #15.

Regarding cache clear on post-update:

  • As \Drupal\taxonomy\Plugin\views\argument_validator\Term is back, I don't think we need it anymore
  • Removal of taxonomy_views_plugins_argument_validator_alter() should not raise cache concerns as the function is checked for existence before is called.
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: claudiu.cristea » Unassigned
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Re #18 we need a cache clear so that sites don't use the deprecate code after updating. An empty post-update should suffice.

Re #15 and adding a constructor with a deprecation - I was thinking \Drupal\taxonomy\Plugin\views\argument_validator\Term was a proper plugin - but it's not. It's not using annotated class discovery - it's hacked in with the alter - so a deprecation in the main section of the code will work just fine.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Rebased after #3039039: Deprecate some procedural functions in taxonomy.module has been committed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me. The three instances of dead code are gone, the entire alter hook that swapped in the dead class is gone, the one remaining non plugin class is empty and deprecated, and there is a post update hook to clear cache. There is nothing left to do here so RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 26c6cd6 and pushed to 9.3.x. Thanks!

Nice to have this fixed before 9.3.x so we have a performance improvement rather than a degradation due to #3039039: Deprecate some procedural functions in taxonomy.module

  • alexpott committed 26c6cd6 on 9.3.x 
    Issue #3221149 by claudiu.cristea, tim.plunkett, guilhermevp, alexpott,...

Status: Fixed » Closed (fixed)

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