Problem/Motivation

On #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, we are updating all base entity fields in entity views data so that they are using Field API for formatting rather than using generic Views handlers.

This issue is about several fields that are currently using a custom 'user' Views handler from the User module:
- NodeViewsData - uid
- UserViewsData - uid
- CommentViewsData - uid

These should all be able to use the basic 'field' plugin instead, and then this custom 'user' plugin can be removed.

Also, the test coverage in
core/modules/user/src/Tests/Views/UserViewsFieldAccessTest.php
should be updated to cover these fields.

Proposed resolution

Change these three fields to use the Field API formatter 'field' instead of the custom formatter. Should also be able to remove the custom formatter from the code base completely.

Remaining tasks

Make a patch.

User interface changes

None.

API changes

Not really.

CommentFileSizeAuthor
#14 views_plugin_user_replaced_field-2470093-14.patch8.26 KBkeopx
#14 interdiff-views_plugin_user_replaced_field-2470093-10-14.txt450 byteskeopx
#10 interdiff-views_plugin_user_replaced_field-2470093-6-10.txt2.46 KBkeopx
#10 views_plugin_user_replaced_field-2470093-10.patch7.61 KBkeopx
#6 views_plugin_user_replaced_field-2470093-6.patch5.02 KBkeopx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented

I'm not assigning myself yet as I have too many issues with my name on them right now, but I can work on this when those get wrapped up (doing a bunch of similar ones), and if nobody else picks this up.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

These should all be able to use the basic 'field' plugin instead, and then this custom 'user' plugin can be removed.

I do agree that we should not support linking a UID to its user, unless you configure it for youself in views.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Discussed with alexpott -- since we put user IDs all over the place (including in cache tags), this one makes sense as a major.

keopx’s picture

keopx
Primary language Basque
Location Basque Country
CreditAttribution: keopx commented
Assigned: Unassigned » keopx
keopx’s picture

keopx
Primary language Basque
Location Basque Country
CreditAttribution: keopx commented
Issue summary: View changes
keopx’s picture

keopx
Primary language Basque
Location Basque Country
CreditAttribution: keopx commented
Status: Active » Needs review
FileSize
views_plugin_user_replaced_field-2470093-6.patch5.02 KB

Test run wrong and I changed values to adding user id and passing correct values to assert

keopx’s picture

keopx
Primary language Basque
Location Basque Country
CreditAttribution: keopx commented

Removed comment

Status: Needs review » Needs work

The last submitted patch, 6: views_plugin_user_replaced_field-2470093-6.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Wow, only one test failure -- that's great!

So. A couple of things to fix on this patch:

a) The 'user' plugin class should be removed in this patch, since it is hopefully not used any more. This is class
Drupal\user\Plugin\views\field\User

b) The test failure... I don't really understand it. The test view config that is being used is in
core/modules/user/tests/modules/user_test_views/test_views/views.view.test_field_permission.yml

For some unknown reason, even in Core this already has the 'uid' field set to be using plugin 'field', and this patch doesn't change that, but the test failure is complaining that plugin 'user' doesn't exist. ??!?

Ah. It thinks that it *depends* on the 'user' plugin... Well, I still don't get it.

keopx’s picture

keopx
Primary language Basque
Location Basque Country
CreditAttribution: keopx commented
Assigned: keopx » Unassigned
Status: Needs work » Needs review
FileSize
views_plugin_user_replaced_field-2470093-10.patch7.61 KB
interdiff-views_plugin_user_replaced_field-2470093-6-10.txt2.46 KB
keopx’s picture

keopx
Primary language Basque
Location Basque Country
CreditAttribution: keopx commented

Remove class

Status: Needs review » Needs work

The last submitted patch, 10: views_plugin_user_replaced_field-2470093-10.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Maybe @dawehner can tell why this fails?

keopx’s picture

keopx
Primary language Basque
Location Basque Country
CreditAttribution: keopx at Isarea commented
Status: Needs work » Needs review
FileSize
interdiff-views_plugin_user_replaced_field-2470093-10-14.txt450 bytes
views_plugin_user_replaced_field-2470093-14.patch8.26 KB

After speak with @dawehner pass local test for test fails

Kudos for @jhodgdon and @dawehner

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Great!

I don't see any problems here. I also tested it manually (just in case!) at least for Node views. Works fine.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Great work! This one looks a lot more straight-forward than the others.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed bfadcb6 on 8.0.x 
    Issue #2470093 by keopx, jhodgdon, dawehner: Views plugin 'user' needs...

Status: Fixed » Closed (fixed)

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