Entity operations for terms are hardcoded in taxonomy terms's overview page

We use a new approach for this in Drupal 8. All entity operations should be provided by the entity type's list builder (which primarily builds entity listings). The list builders allow entity types to provide their own operations, but also invoke the hooks that are necessary for other modules to provide additional operations. The prime example of that in core are probably the content and config translation modules.

Because term listings are more complex–they cannot be listed all at once, but only per vocabulary–we can only use the term list builder for providing operations. We've done this in core once before (I cannot remember where exactly right now).

It's been done before somewhere in a core issue (I remember discussing it with @sun). The reason is that operations handlers could no longer be added to core after code freeze. This is why list builders have become responsible for providing operations links, even though they cannot always provide lists too.

This patch adds destinations.

call parent::__construct() instead of set manually the member variables and change ::getOperations to ::getDefaultOperations.

Duh... Thanks!

Furthemore in ::render() \LogicException is more appropriate, isn't it?

It's all potatoes potatoes to me. I looked it up and \LogicException makes sense indeed.

+++ b/core/modules/taxonomy/src/TermListBuilder.php
@@ -31,36 +31,37 @@
-  public function getOperations(EntityInterface $entity) {
-    $operations = parent::getOperations($entity);
-    foreach ($operations as &$operation) {
-      $operation['query']['destination'] = $this->redirectDestination->get();
+  public function getDefaultOperations(EntityInterface $entity) {
+    $operations = parent::getDefaultOperations($entity);
+
+    $destination = $this->redirectDestination->getAsArray();
+    foreach ($operations as $key => $operation) {
+      $operations[$key]['query'] = $destination;

This change is actually a regression, as it would override any existing GET parameters and we don't want that.

Thanks!

It's unlikely, but the way your patch from #7 does it, is better practice. Basically: don't touch anything you don't have to.

Thanks!

It's unlikely, but the way your patch from #7 does it, is better practice. Basically: don't touch anything you don't have to.

We tried to implement hook_entity_operation_alter() for all entity types in collect contrib and it was not possible to alter tag operations. See #2491307: Add "Capture" operation to all content entities
Works like a charm after applying this patch.

Any reason to introduce non-usable list bulder?
Suppose proper fix is to convert OverviewTerms to list builder

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -30,6 +30,7 @@
+ *     "list_builder" = "Drupal\taxonomy\TermListBuilder",

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -265,28 +272,9 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
       $form['terms'][$key]['operations'] = array(
         '#type' => 'operations',
-        '#links' => $operations,
+        '#links' => $this->termListBuilder->getOperations($term),

+++ b/core/modules/taxonomy/src/TermListBuilder.php
@@ -0,0 +1,77 @@
+  public function render() {
+    throw new \LogicException('This list builder can only provide operations. It does not build lists.');

incredible hack...

See #3 and #4 for why this is done in such a 'hacky' way. In 2014 the decision was made that because of code freeze we could no longer add new entity type handlers. The entire page cannot be converted to a list builder easily or at all, because the page only displays terms for a specific vocabulary, whereas list builders were designed to display all entities of a specified type.

Looks much RTBC to me. (edited) Except: There is no documentation at all that explains why the TermListBuilder is limited to operations only.

Looks much RTBC to me. There is documentation at all that explains why the TermListBuilder is limited to operations only.

Does this mean there is or is not enough documentation?

If there is, do you see any other objections to an RTBC?

I'm not used to documentation verbosity in core.

+++ b/core/modules/taxonomy/src/TermListBuilder.php
@@ -0,0 +1,77 @@
+ * Provides a taxonomy term list builder.
...
+    throw new \LogicException('This list builder can only provide operations. It does not build lists.');

There is zero explanation on why. I would have expected to see some comment similar to your words above:

The entire page cannot be converted to a list builder easily or at all, because the page only displays terms for a specific vocabulary, whereas list builders were designed to display all entities of a specified type.

The technical reason is that we crammed two separate features (list building and operations) in the same interface (ListBuilderInterface) and we shouldn't have. There are numerous reasons why classes wouldn't want to provide lists, but do provide operations.

In practice, the only reason I have seen so far (IIRC) is that we use alternative listing methods. I'm not sure we should explain this in the exception message, as the choice to use these alternative methods lies with the application (the entity type definition) and not with the list builder itself.

However, based on that reasoning, we should perhaps allow lists to be made using this list builder in case someone wants a list of terms across all vocabularies. It would also mean the class complies with the interface and there are no WTFs about application-level decisions.

Thanks for pointing out we can and should improve documentation!

Why is the destination param expected for term operations but not for operations of other entity types? Should we consider adding destination param to (some) operations in EntityListBuilder?

Ran into a similar issue with menu links, so created #2684577: Entity operations for menu links are hardcoded in edit menu form with some changes based on this issue.

Hm i guess it would be 8.x-2.x target meanwhile?

Re-roll for 8.2.x

Did a little bit of digging to answer #25, and found that both term and node operations have been including the destination for a long time. I guess there's no point in trying to change that in this issue.

I queued a retest of #28 (on 8.3.x) to see if anything's changed in 6 months.

Cool. Same two failures. Anyone here able to debug them?

@effulgentsia i am on it.

+++ b/core/modules/forum/src/Form/Overview.php
@@ -61,12 +61,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-        else {
+        elseif (!empty($route_parameters)) {
           $form['terms'][$key]['operations']['#links']['edit']['title'] = $this->t('edit forum');

this change is wrong! edit forum always have params!

@andypost i tried to replicate it.
However exception is at the same line. [Notice] Line 64 of core/modules/forum/src/Form/Overview.php
Tried to reproduce by running test case locally as well.

Can there be anything that is internally calling `Overview.php` of forum module ?

1. +++ b/core/modules/taxonomy/src/TermListBuilder.php
   @@ -0,0 +1,77 @@
   +   * {@inheritdoc}
   +   */
   +  public function getDefaultOperations(EntityInterface $entity) {
   +    $operations = parent::getDefaultOperations($entity);
   +
   +    $destination = $this->redirectDestination->get();
   +    foreach ($operations as $key => $operation) {
   +      $operations[$key]['query']['destination'] = $destination;
   +    }
   +
   +    return $operations;
   +  }
   +
   

   #2767857: Add destination to edit, delete, enable, disable links in entity list builders does this in a generic way, then we wouldn't need this anymore.

2. +++ b/core/modules/taxonomy/src/TermListBuilder.php
   @@ -0,0 +1,77 @@
   +   * {@inheritdoc}
   +   */
   +  public function render() {
   +    throw new \LogicException('This list builder can only provide operations. It does not build lists.');
   +  }
   +
   

   not sure why we need to do this.

Not sure if/why that is needed with the forum operations but what about moving that to hook_entity_operations_alter() and out of the custom controller?

#2767857: Add a list builder with a label and destination query parameters in operation links does this in a generic way, then we wouldn't need this anymore.

But that issue sets the destination to the 'collection' link template. Which wouldn't work for taxonomy terms, since they don't have such a template, and if we wanted to add such a template, it would need to have the {taxonomy_vocabulary} parameter. I guess we could do that by adding a Term::urlRouteParameters() override?

Yeah, but as I commented there, I think that is wrong anyway and we should use the redirect destination API which defaults to the current page. See also #2838968: BlockContentListBuilder should use RedirectDestinationTrait instead of hardcoding the collection link template, any view that displays operation links would have the same problem as term.

This is a bug and should go to 8.2.x. I rerolled #7, that was the last good.

@Berdir,

#2767857: Add a list builder with a label and destination query parameters in operation links does this in a generic way, then we wouldn't need this anymore.

That is a 8.3.x issue and that should account this fix if this will go first. Or we fix here if that goes first.

Removed render() hack.

Re-roll against 8.3.x.
Tests are probably still failing though.

I posted a detailed overview of all related issues that we have around this topic in #1848686-179: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission) (#179 if the link does not work).

As suggested there, I'm closing this issue as a duplicate of that issue as that also has to solve this problem.

I'll try to update that other issue and possibly incorporate the approach that this issue uses with the list builder.

Add the patch for the version Drupal 8.3