Closed (fixed)
Project:
Drupal core
Version:
8.6.x-dev
Component:
field system
Priority:
Major
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
15 Dec 2014 at 10:25 UTC
Updated:
6 Nov 2019 at 13:04 UTC
Jump to comment: Most recent
Problem/Motivation
As discussed in #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency, for reasons of entity translation and entity access enforcement, we need to be rendering all base entity fields in Views using Field API formatters and the entity rendering pipleline, rather than using special-purpose Views handlers or generic Views data formatters. Views data formatters that are not entity-aware or that do not use the standard entity rendering pipeline simply don't get it right, and it's a critical issue because we need to respect entity access for all entity base fields.
This meta-issue exists to track the progress of converting all entity base fields to use Field API formatters.
One problem that occurs in this conversion is that the Field API formatters do not all have the same functionality as the Views generic formatters. For instance, the timestamp Entity field formatter, just renders out the date using format_date(). Views itself allows you to configure basically everything:
$form['date_format'] = array(
'#type' => 'select',
'#title' => $this->t('Date format'),
'#options' => $date_formats + array(
'custom' => $this->t('Custom'),
'raw time ago' => $this->t('Time ago'),
'time ago' => $this->t('Time ago (with "ago" appended)'),
'raw time hence' => $this->t('Time hence'),
'time hence' => $this->t('Time hence (with "hence" appended)'),
'raw time span' => $this->t('Time span (future dates have "-" prepended)'),
'inverse time span' => $this->t('Time span (past dates have "-" prepended)'),
'time span' => $this->t('Time span (with "ago/hence" appended)'),
),
)'
$form['timezone'] = array(
'#type' => 'select',
'#title' => $this->t('Timezone'),
'#description' => $this->t('Timezone to be used for date output.'),
'#options' => array('' => $this->t('- Default site/user timezone -')) + system_time_zones(FALSE),
'#default_value' => $this->options['timezone'],
);
So, we may lose some functionality when we convert. A secondary part of this issue is to avoid that by adding functionality to the Entity Field formatters so that they will match options available currently in the Views formatters that we need to not be using any more.
Proposed resolution
In the discussion on this issue, we decided that the correct resolution was to go ahead and switch to using Entity Field formatting, and live with lost functionality as a first step. This is necessary to enforce entity access on all entity fields, and to support translation properly. Since it involves entity access enforcement, all base fields that are not using entity-aware formatting need to be switched to using entity-aware formatting and this is a critical issue.
As a second step, we want to update the Entity Field formatters so that they support all of the options available in Views formatters. This is not as critical -- we can live with reduced functionality if we have to.
Remaining tasks
Here is a list of the Views formatters that (prior to any of this work) are being used for Entity base fields, what functionality is missing from the corresponding Field formatters, and the plans for replacing them. All of the base fields need to be switched to using entity-aware formatters (these are Critical issues). Replacing the missing functionality is not so Critical.
And there's an issue for making sure entity access is tested:
#2462589: Provide test coverage for access checking of all views fields.
And a follow-up to make sure it's complete:
#2464635: Follow-up: Enable additional tests for base entity field access checking in Views
Outstanding issues (see below for details)
- #2545730: Misuse of formatPlural() in Numeric field prefix/suffix
- #2464635: Follow-up: Enable additional tests for base entity field access checking in Views
Here's the list of formatters to be replaced:
| Plugin, status | Where used | Missing functionality | Issues |
|---|---|---|---|
| Used in: EntityViewsData base class | Raw output (0/1), several options for the yes/no values, and the fully "custom" option. |
- #2395613: Make it possible to configure the output of a boolean field on the formatter level updated the formatter. - #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency makes the base EntityViewsData use it. |
|
|
DONE! |
Used in EntityViewsData. Note that while NodeViewsData and other data classes have specific formats added as pseudo-fields, they are only for arguments (contextual filters), not field output, so we don't have to worry about them here. | Selecting a date format, custom date formats, time zone selection, time ago |
- #2399211: Support all options from views fields in DateTime formatters updated the formatter. - #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter makes entities use it. Major |
|
- language: \Drupal\views\Plugin\views\field\LanguageField, used in EntityViewsData - node_language: \Drupal\node\Plugin\views\field\Language, used in NodeViewsData - user_language: \Drupal\user\Plugin\views\field\Language, used in UserViewsData - taxonomy_term_language: \Drupal\taxonomy\Plugin\views\field\Language, used in TermViewsData |
Link to entity, display language using native name | #2384863: Translation language base field handler should use views field handler, provide unified options adds the formatting options, updates EntityViewsData to use formatter, and removes all three of these entity-specific plugins. | |
|
|
Used in EntityViewsData | None. | #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency changes EntityViewsData to use field formatting. |
| numeric: \Drupal\views\Plugin\views\field\Numeric - in progress [Critical part is done] |
- Used in EntityViewsData both for regular numeric fields and Entity Reference fields. - Used in FileViewsData for fields on file_usage table: id, count [not base entity fields!] - Used in CommentViewsData for fields on comment_entity_statistics table: comment_count, last_comment_uid [not base entity fields!] |
Plural formatting, such as "1 thing" vs. "2 things" |
- #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency changed EntityViewsData to use field formatting. - Add settings for plural: #2449597: Number formatters: Make it possible to configure format_plural on the formatter level, now closed as a duplicate of #2545730: Misuse of formatPlural() in Numeric field prefix/suffix. This depended on #2453761: Views numeric formatter's plural formatting setting incompatible with many languages, which in turn depended on #2454859: Not possible to format plural an already translated string; both of those are done. - Cannot convert non-base fields to use field formatting, so the rest are not issues. |
|
|
Used in EntityViewsData both for regular text fields and Entity Reference fields. Used in FileViewsData for fields on file_usage table: module, type [not base table!] Used in CommentViewsData for fields comment_field_data.field_name, comment_entity_statistics.field_name [not base table!], comment_entity_statistics.entity_type [not base table!] |
None. |
- #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency changes EntityViewsData to use field formatting. - #2455131: Field comment_field_data.field_name should be using Field API formatter for CommentViewsData use on comment_field_data.field_name - Database fields that are not on base tables cannot use field formatting, so the others are not issues. |
|
|
Used in EntityViewsData, UserViewsData (signature field) | None. |
- #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency changes EntityViewsData to use field formatting. - #1548204: Remove user signature and move it to contrib is for where it is used in UserViewsData for the Signature field. |
|
|
These are used in NodeViewsData: - node (fields: nid, title) - node_type (fields: type) [entity bundle... not sure what to do with this, but it is probably OK?] - node_link (pseudo-field: view_node) - node_link_edit (pseudo-field: edit_node) - node_link_delete (pseudo-field: delete_node) - node_path (pseudo-field: path) - node_bulk_form (pseudo-field: node_bulk_form) - node_revision (field: title on node_revision) - node_revision_link (pseudo-field: link_to_revision on node_revision) - node_revision_link_revert (pseudo-field: revert_revision on node_revision) - node_revision_link_delete (pseudo-field: delete_revision on node_revision) |
Probably not. |
- #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency removes node handler for base table title field. - Post-2342045, the remaining plugins in NodeViewsData for fields are: -- node_type (bundle field, not sure what to do with it, as it's not really an entity field). -- Various link-related fields/plugins: node_link, node_link_edit, node_link_delete, node_path, node_revision_link, node_revision_link_revert, node_revision_link_delete. See #2322949: Implement generic entity link view field handlers. -- node_bulk_form -- this appears to be entity-aware so is probably OK -- node_revision: #2456599: Field node_field_revision.title needs to use an entity-aware formatter in Views Critical [done] - Post-2322949 and 2456599, the remaining link plugins that are still in NodeFieldData are: -- node_path -- node_bulk_form -- node_revision_link -- revert_revision -- delete_revision -- search_score These are probably fine to leave as-is. Search is not in the base entity tables. Revision links are not covered by the generic entity link plugins. Node path... probably not necessary but not really horrible to have. And the bulk form is its own thing too. So, this is done! |
|
|
Here's the list: - user (fields: NodeViewsData - uid, UserFieldData - uid, CommentViewsData - uid) - user_name (field: UserFieldData - name) - user_mail (field: UserFieldData - mail) - user_link_edit (pseudo-field: UserFieldData - edit_node, which despite the name is actually a link to edit the user) - user_link_cancel (pseudo-field: UserFieldData - cancel_node, which despite the name is actually a link to cancel the user) - user_data (pseudo-field: UserFieldData - data) - user_bulk_form (pseudo-field: UserFieldData - user_bulk_form) - user_roles (UserFieldData - roles_target_id in user__roles table - not on a base table) - user_permission (UserFieldData - permission pseudo-field in user__roles table - not on a base table) |
Probably not. |
- Post-2342045, the base field plugins remaining in UserViewsData (as well as Node/Comment where they reference User) on base tables are: -- user_mail: see #2456691: User email field need to use Field-Entity-aware formatters in Views Critical [done] -- user_name: see #2454145: Replace user_name handler with Field API formatter Major [done] -- user: see #2470093: Views plugin 'user' needs to be replaced with entity-aware 'field' plugin Major [done] -- various link fields/plugins: view_user, user_link_edit, user_link_cancel - see #2322949: Implement generic entity link view field handlers but not sure if that will actually fix these. Probably not critical anyway since the worst is someone gets a link they shouldn't and it 403s on them. -- user_data: This gets data from the "user_data" service. See #2456695: Remove or fix user_data Views integration: make sure it does proper entity access/translation and filtering, or get rid of it -- decided this was OK as-is. -- user_bulk_form -- this appears to be entity-aware so is probably OK |
|
|
- aggregator_title_link: in AggregatorFeedViewsData and AggregatorItemViewsData for the title fields - aggregator_xss: in AggregatorItemViewsData for the author and description fields |
The XSS plugin derives from the Views XSS base plugin. We would need to have XSS filtering available on a Field UI formatter. |
- #2456701: Replace aggregator_title_link Views formatter with Field API formatter for aggregator_title_link - Major [done] - #2455149: Aggregator xss fields should be using Field/Entity formatters for aggregator_xss formatter - Major [done] |
|
- comment: Used in CommentViewsData for fields subject, cid - comment_username: Used in CommentViewsData for field name - comment_link: Used in CommentViewsData for pseudo-field view_comment - comment_link_edit: Used in CommentViewsData for pseudo-field edit_comment - comment_link_delete: Used in CommentViewsData for pseudo-field delete_comment - comment_link_approve: Used in CommentViewsData for pseudo-field approve_comment - comment_link_reply: Used in CommentViewsData for pseudo-field replyto_comment - comment_depth: Used in CommentViewsData for field thread - comment_last_timestamp: Used in CommentViewsData for field last_comment_timestamp [this is on comment_entity_statistics, not on base entity table] - comment_ces_last_comment_name: Used in CommentViewsData for field last_comment_name [not base table] - comment_ces_last_updated: Used in CommentViewsData for field last_updated [not base table] |
Probably not. |
- Post-2342045, the Comment module base field plugins remaining in CommentViewsData on base tables are: -- 'comment', used on comment_field_data.subject, comment_field_data.cid; 'comment_depth' used for comment_field_data.thread. Issue: #2456705: Comment views field handlers need to be replaced with field/entity aware handlers Critical [done] -- 'comment_username' used for comment_field_data.name - Issue: #2454163: Replace comment_username handler with generic views handler Critical [done] -- various link fields/plugins: comment_link, comment_link_edit, comment_link_delete, comment_link_approve, comment_link_reply - see #2322949: Implement generic entity link view field handlers but not sure if that will actually fix these. Probably not critical anyway since the worst is someone gets a link they shouldn't and it 403s on them. |
|
|
- block_content: Used in BlockContentViewsData for field id on base table, info on field data table - block_content_type: Used in BlockContentViewsData for field type on field data table |
Probably not. | #2456707: Block Content views field handlers need to be replaced with entity-aware handlers | |
|
|
- file: In FileViewsData for fields fid, filename - file_uri: In FileViewsData for field uri - file_filemime: In FileViewsData for field filemime - file_extension: In FileViewsData for field extension - file_size: In FileViewsData for field filsize - file_status: In FileViewsData for field status |
Probably not? | #2456709: File views handlers need to be replaced with entity-aware formatters Critical [done] |
|
|
Used in TermViewsData: - term_edit_link - field edit_term - taxonomy - field name |
TBD |
- Post-2342045, the remaining plugins in TermViewsData for fields are: -- 'taxonomy', used in taxonomy_term_field_data.name #2456713: Custom taxonomy field views handler needs to be replaced with generic Field API handler Critical [done] -- Various link-related fields/plugin: term_link_edit. See #2322949: Implement generic entity link view field handlers but not sure if that will actually fix this. Probably not critical anyway since the worst is someone gets a link they shouldn't and it 403s on them - 'term_name' - which derives from an entity-aware plugin, so it's OK |
|
|
- content_translation_link (Used in pseudo-fields called translation_link in: NodeViewsData, UserViewsData, BlockContentViewsData, CommentViewsData, TermViewsData)
- xss (used in fields: NodeViewsData - revision_log, AggregatorFeedViewsData - description, BlockContentViewsData - revision_log) - search_score (fields: NodeViewsData - node_search_index.score) [not an Entity field, cannot use Field rendering] entity_label (fields: FileViewsData - file_usage.entity_label) [not an Entity field, cannot use field rendering] |
XSS filtering |
- content_translation_link -- see #2322949: Implement generic entity link view field handlers Probably not critical anyway since the worst is someone gets a link they shouldn't and it 403s on them - AggregatorFeedViewsData use of xss - #2455149: Aggregator xss fields should be using Field/Entity formatters - Other uses of xss - #2455153: Switch revision log views fields to use 'field' formatter |
User interface changes
Entity fields will be formatted using Entity rendering in Views. Entity fields in and out of Views will have more formatting options available, and they'll be consistent with what is available in Views for base data fields.
API changes
None.
Comments
Comment #1
dawehnerAdded the parent issue.
Comment #2
gábor hojtsyI don't think it would be a problem to have the options as long as its consistent. I don't think people building the views even necessarily understand why base fields would have reduced features in views. Are you concerned for too many options appearing?
Comment #3
dawehnerWell, I'm more concerned that the amount of features we have in views aren't visible for base fields, so pretty much all of them.
On the other hand I could imagine that people complain about the amount of options this will add to the field UI.
Comment #4
gábor hojtsyI think those are the options you get for date fields, then why not provide them for base date fields :)
Comment #5
larowlanI think we discuss on a case by case basis. For timestamp I've always envied what views provides relative to field api, so that's a +1 from me.
Comment #6
jhodgdonI think maybe some of the comments here are misunderstanding the question in this issue, or else I am?
So... I think the problem is that the proposal on the parent issue is that instead of using the Views field handler classes to format the Base fields of an entity (which is what we are doing now), we would switch to doing what we do for Field UI fields on the entity: we would use Entity/Field Formatters. (And the reason we need to do that is because right now, the base Views field handlers do not understand that the data they're getting is part of an entity, so it's not being access checked or translated, it's just "I got this database field, let's format it".)
However, the Views field handler classes have more formatting options than the Entity/Field formatters. So the question on this issue is basically: is it OK to lose all the nice options? And if not... what can we do about it?
Right?
Comment #7
larowlanI see there is a second option, to port the views extra bits to the field api
Comment #8
jhodgdonYes, indeed. I think if the Views formatters for some kind of data have more options than what the basic Field API has, then we should definitely consider porting them to either existing or new Field API Formatters for the Fields that hold that type of data.
I'm not entirely certain this will be possible or appropriate for *all* the settings, but it seems likely that it would be for at least *most* of the settings. If we lose a few possibilities/settings from Views, we may have to live with that; losing a lot is probably not as desirable.
The case in the issue summary here is about time/date. It seems like the stuff shown there would all be good to have on the Field API formatter for dates. It might mean making a new Formatter class as an alternative to whatever ones exist now for DateTime fields, but it seems well within the realm of both Possible to Do and Good to Have.
Comment #9
dawehnerLet's promote it for now, it blocks a critical kinda
Comment #10
plachComment #11
catchIf I've got the options right, then:
- I'd be fine losing features in Views to resolve the critical (and completely dropping the custom formatters would be great clean-up as a bonus).
- I'd be fine adding those features back to the Field UI formatters separately, but it could use UI review and shouldn't/doesn't have to block release at all.
Comment #12
dawehnerEven I kinda hate to loose features temporarily I think that is fair. I hope
this UI review won't say that the options are too advanced, because they got added due to the demand of many people.
Can we close this issue now and mark it as "decided"?
Comment #13
dawehnerSo decided seems to be mean RTBC
Comment #14
jhodgdonI think if @catch and @dawehner agree (and no one here seems to disagree), we can call it "decided". This issue is policy so no patch.
Comment #16
jhodgdonI'm going to reopen this issue as a meta issue, where we can make sure all of the needed fields have been updated according to this policy. Accordingly, downgrading the Priority.
Updating the summary with a survey of the fields we need to address.
Comment #17
jhodgdonCompleting the survey... the summary was incomplete as of the last comment.
After completing this table, it strikes me that a unified plugin to make entity links would probably reduce the custom plugins a lot. I'm not convinced currently that all of the link-making plugins specific to Node and other modules are doing the right thing with regard to display language. We'll have to check on that.
Some of the other plugins, like the ones for the node title, can probably be removed in favor of just using field formatting, I think. I believe the main reason these exist now is so they can be made into a link to the entity.
Anyway... at least there is a list now.
Comment #18
dawehnerAmazing work on the issue summary!!!!
Comment #19
jhodgdonSome updates.
As noted by dawehner on a related issue, fields that are not on the base/data entity tables cannot be rendered using Field rendering. So, noting that in the table in the issue summary.
Comment #20
jhodgdonForgot about this related issue. Added it to the summary table in various places.
Comment #21
gábor hojtsyI don't think the title is appropriate anymore to the META-ness of this? In many cases, its not about formatters.
Comment #22
jhodgdonHow about this?
Comment #23
jhodgdonAs Gabor pointed out on related issues, we need to figure out which of these fields are critical to convert now, and make issues for them. The reason is that if we don't do them now, they'll need upgrade paths (affect stored views).
By the way, I'm not sure why this issue has an "entity storage" tag. It's about Views, how is that "entity storage"? Maybe I don't understand the tag though so leaving it for now.
Comment #24
gábor hojtsyBumped to critical given there are children which are critical.
Comment #25
xjmAgreed, we can downgrade this once we're sure everything that needs to be critical (and upgrade path blocking) is tagged and filed or complete.
Also retitling because the word "policy" makes me scan past it. :P
Comment #26
gábor hojtsyFixing intro of issue summary.
Comment #27
jhodgdonThe issue that was marked as this one's parent is now marked as a child. We don't want any time paradoxes of being your own grandparent!
Comment #28
dawehnerHAHA, yeah I have seen that but just could not motivate me to fix it :)
Comment #29
catchUsing field API formatters seems critical since it'll be a pain to sort out the upgrade path afterwards.
Not losing any configuration options not so much - we could add those in major issues before or after release no?
Comment #30
dawehnerYeah its not part of the criticality. I would vote to move this issue to major, once #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency and #2385443: Test that base entity fields on views respect field level access is in.
Comment #31
gábor hojtsyBut this is critical because unless we solve it for the rest of the base fields that #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency does not cover, we end up not resolving #2385443: Test that base entity fields on views respect field level access either for all base fields, right?
Comment #32
dawehnerWe can indeed switch to the formatters already, but I'd hate to not longer have all the options of views field handlers, but I agree with @catch that
those options are not critical.
So for me the way to do would be, fixing the other two critical, then add the settings to the formatters and switch them over. If time is not enough, we skip the
settings part of it before the RC.
Comment #33
gábor hojtsyCan we resolve #2385443: Test that base entity fields on views respect field level access across all base fields without switching all base fields over?
Comment #34
dawehnerNo we can't.
Comment #35
gábor hojtsyOk, so we either need to make that turn over the rest of the base fields without the formatter updates all at once or make the rest of the subissues happen here.
Comment #36
dawehnerYeah exactly. The first bit is a backup plan, IMHO
Comment #37
dawehnerAdded an issue: #2449597: Number formatters: Make it possible to configure format_plural on the formatter level
Comment #38
jhodgdon#2425193: Convert the signature field in user_field_data to the ordinary views field is closed as duplicate of #1548204: Remove user signature and move it to contrib, updating summary.
Comment #39
jhodgdonUpdating summary and filing new issues that were missing. New issues filed were listed with this one as Parent. Here's a list if you want to follow them or better yet work on them:
#2455125: Update EntityViewsData use of generic timestamp to use Field API formatter
#2455131: Field comment_field_data.field_name should be using Field API formatter
There are more to be evaluated, but getting this in for now...
Comment #40
jhodgdonFiled #2455149: Aggregator xss fields should be using Field/Entity formatters for Aggregator fields, and #2455153: Switch revision log views fields to use 'field' formatter for XSS filtered formatting.
Comment #41
xjmThanks @jhodgdon for getting those filed!
Now that #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency and #2385443: Test that base entity fields on views respect field level access are in, I looked over the list of remaining work in the summary. As far as I can tell, none of the outstanding work is critical, nor are the feature regressions (though certainly a lot of it is major). It seems like for the most part, the remaining issues are either Views not leveraging the formatter functionality for for specific fields (and therefore some fields are not properly rendered or translated, do not support in-place editing, etc.). But all the access checking issues are resolved, correct?
Specifically:
Among the plugins we haven't checked yet, do we think any of them have potentially critical issues in not using Entity API formatters?
Comment #42
dawehnerWell, we have still tons of handlers ... and every usage blocks translation support to work.
So for example we still have no really support for links to revisions, or even just displaying the username in a proper way. You could all consider this as bugs,
but still, we are bypassing the entity access API for fields not using the formatters.
Comment #43
gábor hojtsyLooks like the key sentence then is we are bypassing the entity access API for fields not using the formatters. If we considered the prior ones critical, then why not these?
Comment #44
dawehnerExactly, on top though there is also the argument of translatability.
Comment #45
jhodgdonIndeed. Any base field that is not using an entity-aware formatter is bypassing entity access (critical) and has completely broken translatability (probably also critical; at least major; but it doesn't matter because the fix for entity access is the same fix: use the Field API).
The tests/patches so far have fixed some of these critical sub-issues, but not all, and the remaining ones are still critical. The loss of functionality patches are not critical, but we're not down to just those yet.
I have to finish going through this list and filing some more issues. Plan to do that shortly...
Comment #46
xjm@dawehner, @Gábor Hojtsy, thanks! It wasn't clear to me that entity access was still being bypassed for these fields with the two patches that already went in. So let's bump each of those outstanding issues to critical where access control is bypassed.
A summary and title update would also be great, because I did read the summary very carefully and didn't understand that these fields were still bypassing access control.
Comment #47
jhodgdonUpdating summary. I still need to go through the remaining "needs evaluation" items and evaluate them. Plan to get to that today if possible.
Comment #48
jhodgdonI elevated a couple of the child issues to Critical, and filed a new one. And added a few notes to the summary table. Running out for a bit but I got through evaluating a few more and changed status accordingly... a bit more to do later today.
Comment #49
jhodgdonGot through the User plugins. #2456695: Remove or fix user_data Views integration: make sure it does proper entity access/translation and filtering, or get rid of it is pretty scary -- someone please tell me I'm wrong?!? A few more left to evaluate...
Comment #50
jhodgdonGot through Aggregator, Comment, Block Content, File... almost through the eval...
Comment #51
jhodgdonOK, finished filling in all the issues, phew! I guess I single-handedly upped the critical count by about 10 today. However, as xjm said in IRC, they're small, known, doable criticals rather than this big "Who knows what we need to do" critical meta. I think I got them all. One of the new ones already even has a patch. ;)
Comment #52
jibranAFAIK all the children of critical meta should be major until we have 2 children. Why are they all critical?
Comment #53
jhodgdon@jibran #52 - because xjm asked me to make them all critical. ;)
Comment #54
dom. commentedHi,
I have a concern with content types displayed in views that seems related. Please see: #2458223: Duplicated field handlers in field UI for some base table fields
Comment #55
jhodgdonUpdate status of meta.
Comment #56
xjm@jibran re: #52:
Any issue that has critical implications on its own should be filed as critical. See the definition of a critical issue. (The one exception would be e.g. the router or CMI conversions where there were hundreds of children that were all blockers to removing the legacy system, and it simply wasn't reasonable to mark them all as critical and still have a usable issue queue due to their small scope individually. That's more of an issue management decision.) But during this phase of the release cycle especially, really every issue which is critical should be filed as such so that the extent of our technical debt is clear.
There is no rule like "Children of issue priority X always have priority Y" or "Children of issue priority X have priority Y when there are N of them". A critical meta can have any number of non-critical children, or vice versa.
Issues where there is unexpected disclosure of field data due to access checking being inconsistently skipped for some Views fields but not others are covered by this point in the criteria for critical issues:
@jhodgdon went through the children and figured out which issues those were (which is awesome and needed work), per my request in #46.
Hope that helps clarify. :)
Comment #57
webchickPer xjm, the subset of issues in this meta that are critical would make good candidates for Critical Office Hours. Tagging.
Comment #58
jhodgdonAdding link to new testing issue #2462589: Provide test coverage for access checking of all views fields.
Comment #59
effulgentsia commentedI just now tagged the relevant subset, so removing the tag from this meta.
Comment #60
jhodgdonComment issues have been split up. And updating status.
Comment #61
jhodgdonNew child issue for test follow-up #2464635: Follow-up: Enable additional tests for base entity field access checking in Views
Comment #62
jhodgdonUpdated meta status...
Comment #63
jhodgdonI just went through the existing Entity ViewsData classes to see what is left. There was one that got overlooked -- the 'user' plugin, used in UserViewsData, NodeViewsData, CommentViewsData. Added to meta and commented on an issue that was added to the meta that was addressing the user_name plugin (no idea how they're different or why/if we need either one, most likely not...).
Comment #64
jhodgdonOne more update for the 'user' plugin:
#2470093: Views plugin 'user' needs to be replaced with entity-aware 'field' plugin
Comment #65
xjmI discussed #2470093: Views plugin 'user' needs to be replaced with entity-aware 'field' plugin a bit with both @dawehner and @alexpott. Based on @jhodgdon's review, I think all the access-related bugs from this have been fixed! So I am downgrading this to major. Edit: Other than #2456599: Field node_field_revision.title needs to use an entity-aware formatter in Views, I meant, which is already critical on its own.
The next step is to finish the remaining bugs and fix our multilingual support.
Great work everyone! And thanks especially to @jhodgdon for keeping things on track here.
Comment #66
xjmAdding a short list of the remaining issues to the summary.
Comment #67
xjmComment #68
xjmComment #69
jhodgdonWow, lots of progress at DDD! Updated summary. Just a few things left to do really...
Comment #70
mpdonadio{#2399211] and #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter are really just waiting on consensus and/or input from the Views maintainers on what we want to do about the timeago formats, then I can wrap them up.
Comment #71
jhodgdonUpdating issue summary. Wow, lots of progress! The numeric and time stamp issues are all that are left. Great work everyone!
Comment #72
jhodgdonUpdating meta status
Comment #73
mpdonadioUpdated IS now that #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter is in.
Comment #74
jhodgdonUpdate: For Numeric fields, we closed #2449597: Number formatters: Make it possible to configure format_plural on the formatter level as a duplicate of #2545730: Misuse of formatPlural() in Numeric field prefix/suffix, so updating the summary.
Comment #81
joseph.olstadComment #82
amateescu commented#2464635: Follow-up: Enable additional tests for base entity field access checking in Views and #2545730: Misuse of formatPlural() in Numeric field prefix/suffix are the only two remaining issues that still need some work, but I think this meta has run its course so I'm going to close it :)