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:
- node_field_data.uid
- comment_field_data.uid
- users_field_data.uid
Also, two fields that are using other custom user Views handlers:
- users_field_data.name ('user_name' plugin)
- users_field_data.mail ('user_mail' plugin)
Proposed resolution
Change these fields to use the Field API formatter 'field' instead of the custom formatter. Should also be able to remove the custom formatters from the code base completely.
Remaining tasks
Commit it
User interface changes
None.
API changes
Not really.
Comment | File | Size | Author |
---|---|---|---|
#26 | 2456691-25.patch | 3.87 KB | YesCT |
#26 | interdiff.tests_.mail_.txt | 605 bytes | YesCT |
#26 | 2456691.plus_.2462589.patch | 22.63 KB | YesCT |
#26 | 2462589.tests-only.with_.mail_.patch | 18.76 KB | YesCT |
#25 | 2456691-25.patch | 3.87 KB | dawehner |
Comments
Comment #1
jhodgdonComment #2
andypostComment #3
andypostInitial code to remove mail plugin, the only problem here that link to user functionality setting could be achieved by adding a UID field to view and overriding the field with link.
Suppose there should be an option in views like provide a link to entity for each entity fleld.
Also I found that there's 2 UID fields that could be added - probably separate issue
Comment #4
andypostNow clean-up name usage
Comment #6
andypostmissed interdiff
Comment #7
rteijeiro CreditAttribution: rteijeiro commentedFixed what testbot is complaining about. Not sure if it's right @andypost, let me know otherwise.
Comment #10
dawehnerI'm sorry but this sounds like a duplicate of #2454145: Replace user_name handler with Field API formatter, but without adding a new formatter ...
...
Let's rescope the issue to just be on emails
Comment #11
andypost@dawehner so #3 does that, do we need to unpublish other patches?
Comment #12
dawehnerWhere did that option went to?
Comment #13
andypostCurrently only string and mailto formatters are supported, to make the field as link:
1) use views aboility to override
2) or inherit the plugin from field
Comment #14
dawehnerAh, so there is a specific mailto formatter. +1 for #3
Comment #15
andypostSo #3 still applies cleanly
Comment #16
dawehner#3 looks fine for me!
Comment #17
webchickSo we're fixing a bug here, bad enough to be critical and block D8's release, but we're not adding any test coverage for it. Feels a bit strange.
Talked about this in IRC w/ xjm and dawehner. While in most cases, #2385443: Test that base entity fields on views respect field level access is sufficient to check for this condition (after all, it's highly unlikely that once we've turned something into a field we would un-field it again), this particular field contains sensitive user information so would likely result in a SA if it were to break again.
So let's get a small test here to ensure this stays working as expected.
Comment #18
dawehnerStarted to write some generic test which can be reused for this issue and potentially all others.
Comment #19
dawehnerDecided to split of the access checking into its own issue, in order to have a more fundamental / wide spread effort to check access.
See #2462589: Provide test coverage for access checking of all views fields.
Comment #20
jhodgdonRE #10, rescoping: did you file another issue to take care of the rest of what is in the summary, and update the meta-issue parent? The summary still says it covers a bunch of fields. We need to cover them *somewhere* and the other issue mentioned in #10 doesn't cover them I think?
Comment #21
dawehnerWe could add all the fields in here, for all the entity types and comment those out, which fail, as they aren't yet converted.
For example the mail field, which is part of this issue, fails currently on the provided test.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedPer #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.
Comment #23
dawehnerSo I'm curious whether we can get the issue in and add the test coverage as part of the other patch (#2462589: Provide test coverage for access checking of all views fields.)
Comment #24
alexpottSo apparently the patch in #3 is rtbc - can the latest patch be rtbc please? Also I think it is better to introduce a generic test. Access tests are hard to get right and doing it well once makes sense to me.
Comment #25
dawehnerComment #26
YesCT CreditAttribution: YesCT commentedso, it seems like the tests from the other issue will cover this.
to demonstrate, here are some combined patches.
but 25 without the tests is the rtbc'able committable thing.
Comment #28
dawehnerThank you for posting a test only patch but well.
Set the patch of comment NR3. finally back to RTBC again.
Comment #29
YesCT CreditAttribution: YesCT commentedI think this shows what @alexpott asked for, that the generic tests will cover this.
Comment #30
alexpottWe've got the critical followup to add tests so committed the test free patch. Committed 26984ad and pushed to 8.0.x. Thanks!