Custom taxonomy field views handler needs to be replaced with generic Field API handler

Giddyup

changed definition and updated test/default views
This alters the default taxonomy view, so will need a change record for those running d8 sites.

and deletes the plugin

Schema issues
We have existing tests which use this field

1. +++ b/core/modules/taxonomy/src/TermViewsData.php
   @@ -94,7 +94,8 @@ public function getViewsData() {
   +    $data['taxonomy_term_field_data']['name']['field']['field_name'] = 'name';
   

   We don't need this line.

2. +++ b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.taxonomy_default_argument_test.yml
   @@ -94,7 +94,6 @@ display:
              hide_empty: false
              empty_zero: false
   -          link_to_taxonomy: true
              relationship: none
              group_type: group
              admin_label: ''
   

   ... Do we link the name by default? I don't think so.

3. +++ b/core/modules/taxonomy/config/schema/taxonomy.views.schema.yml
   @@ -91,17 +91,6 @@ views.field.term_link_edit:
   -    convert_spaces:
   -      type: boolean
   -      label: 'Convert spaces in term names to hyphens'
   -
   

   What do we do about this?

Fixes for #7 and #8

wizard changes

schema/wizard changes

Fixes #14 note that the changes in views.view.test_taxonomy_node_term_data.yml were wrong, that was an argument plugin, so reverted.

1. +++ b/core/modules/taxonomy/src/Plugin/views/field/TermName.php
   @@ -0,0 +1,56 @@
   +        if (!empty($result->{$this->aliases['name']})) {
   

   I'm pretty convinced that this line doesn't have any test coverage. Nothing is adding this alias if I see that correctly in this file. If you want to the field alias of the current file, use $this->field_alias

2. +++ b/core/modules/taxonomy/src/Plugin/views/field/TermName.php
   @@ -0,0 +1,56 @@
   +          $result->{$this->aliases['name']} = str_replace(' ', '-', $result->{$this->aliases['nid']});
   

   huch, we replace the values of the name with the nid?

I'm pretty convinced that this line doesn't have any test coverage.

Given point 2, I think we can say 110% there are not tests ;)

can't see myself having time for tests today, but will come back for them

Given point 2, I think we can say 110% there are not tests ;)

We need meta tests!

Well, as this test clearly shows, it doens't work.

Note: $result->... is not used anymore, so the entire implementation doesn't work :)

Does it work in HEAD? if not should we be splitting the convert spaces functionality into a non-critical followup?

It works in HEAD, because there the field is loaded as part of the query. Once you use the entity API field, you no longer do.

Per #2393339-57: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, tagging for critical office hours, but if there's a reason to not have this particular child issue in that list, please untag it.

+++ b/core/modules/taxonomy/src/Plugin/views/field/TermName.php
@@ -0,0 +1,56 @@
+   * {@inheritdoc}
+   */
+  public function preRender(&$values) {
+    if ($this->options['convert_spaces']) {
+      foreach ($values as $result) {
+        if (!empty($result->{$this->field_alias})) {
+          $result->{$this->field_alias} = str_replace(' ', '-', $result->{$this->field_alias});
+        }
+      }
+    }
+  }

So what you can do is to override ::getItems, see https://www.drupal.org/files/issues/interdiff_11573.txt

will poke at this later today

So this works, but the test still fails, I think the way the display is changed doesn't stick
Any ideas?

$this->options['convert_spaces'] is always false in the test.

This should be it, feel free to upload a patch.

looks ready to me, thanks @dawehner

We have to add a TaxonomyTermViewsFieldAccessTest but it should be pretty easy to do, given that the parent already does all the hard logic.

Overall +1 for the patch.

+++ b/core/modules/taxonomy/src/Plugin/views/field/TermName.php
@@ -0,0 +1,60 @@
+ * A field that displays the taxonomy term name.

It would be great if we could improve this in next reroll.

Discussed with @webchick, @effulgentsia, and @alexpott. We agreed that this issue is critical because Views should respect access restrictions for taxonomy term field content and there are common usecases for this.

Also +1 for #31; thanks @dawehner!

working on test

And a test

Awesome this is ready. Just few minor things.

1. +++ b/core/modules/taxonomy/config/schema/taxonomy.views.schema.yml
   @@ -91,13 +91,9 @@ views.field.term_link_edit:
   +views.field.term_name:
   +  type: views.field.field
      mapping:
   ...
        convert_spaces:
          type: boolean
          label: 'Convert spaces in term names to hyphens'
   
   @@ -169,3 +165,11 @@ views.relationship.node_term_data:
   +
   +views.field.term_name:
   +  type: views.field.field
   +  label: 'Term name'
   +  mapping:
   +    convert_spaces:
   +      type: boolean
   +      label: 'Convert spaces in term names to hyphens'
   

   Why we have this twice?

2. +++ b/core/modules/taxonomy/src/Plugin/views/field/TermName.php
   @@ -0,0 +1,60 @@
   + * A field that displays the taxonomy term name.
   

   Can we please add some description explaining the use case and all that?

3. +++ b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_groupwise_term.yml
   @@ -27,7 +27,10 @@ display:
   +            link_to_entity: true
   
   +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_groupwise_term_ui.yml
   @@ -27,7 +27,10 @@ display:
   +            link_to_entity: true
   

   There is no convert_spaces setting some views export gone wrong maybe or manual edit? I think we have to re-export these views.

4. +++ b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_term_name.yml
   @@ -128,9 +128,11 @@ display:
              convert_spaces: false
   

   Shouldn't this be under settings now after this change?

+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyViewsFieldAccessTest.php
@@ -0,0 +1,68 @@
+
+    $this->assertFieldAccess('taxonomy_term', 'name', 'Majorly random');
+    $this->assertFieldAccess('taxonomy_term', 'name', 'Semi random');
+    $this->assertFieldAccess('taxonomy_term', 'name', 'Not really random');

Can you expand the test coverage with ID, UUID and what not, please?

No need for a change record here, functionality is equivalent.
Fixes #36 and #37

Re #36.4 - no its part of the plugin options, not the formatter settings.

Thanks for the fixes.

Thanks for the fixes.

Thanks

Doesn't fail locally, opened #2469493: Random fails in Drupal\comment\Tests\Views\CommentUserNameTest?

+++ b/core/modules/comment/config/schema/comment.views.schema.yml
@@ -4,17 +4,6 @@ views.argument.argument_comment_user_uid:
-views.field.comment:
-  type: views_field
-  label: 'Comment'
-  mapping:
-    link_to_comment:
-      type: boolean
-      label: 'Link this field to its comment'
-    link_to_entity:
-      type: boolean
-      label: 'Link field to the entity if there is no comment'
-

+++ b/core/modules/comment/src/CommentViewsData.php
@@ -27,9 +27,6 @@ public function getViewsData() {
     $data['comment_field_data']['subject']['help'] = t('The title of the comment.');
-    $data['comment_field_data']['subject']['field']['id'] = 'comment';
-
-    $data['comment_field_data']['cid']['field']['id'] = 'comment';
 
     $data['comment_field_data']['name']['title'] = t('Author');

I have no idea why those changes landed as part of that patch ... you've uploaded the wrong patch :)

Now with correct patch.

larowlan--

whoops

well that was the long way round, but we're back :)
can't believe I opened a 'random test fails' issue and it turned out I'd picked the wrong file in the 'browse' window.
pebkac

And 20 comments later we are back to RTBC.

Moral: Never RTBC an issue based only on interdiff. :P

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 418497e and pushed to 8.0.x. Thanks!