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.
We have several base entity fields in Views data right now that are using the 'xss' generic data handler. We need them to use the field API 'field' formatter instead.
Proposed resolution
Switch these two fields to use the 'field' formatter:
- NodeViewsData - for the revision_log field
- BlockContentViewsData - revision_log field also.
#2455149: Aggregator xss fields should be using Field/Entity formatters does something similar for Aggregator fields.
Remaining tasks
Make a patch.
User interface changes
None.
API changes
Not really.
Comment | File | Size | Author |
---|---|---|---|
#19 | interdiff-2455153-19.txt | 582 bytes | damiankloip |
#19 | 2455153-19.patch | 5.99 KB | damiankloip |
#12 | interdiff-2455153-12.txt | 939 bytes | damiankloip |
#12 | 2455153-12.patch | 6.02 KB | damiankloip |
Comments
Comment #1
dawehnerMh, I wrote that in some other issue as well, but I think we don't need that, because we have auto-escaping these days ... It would be great if someone with more knowledge about the auto-escaping system could comment.
Comment #2
jhodgdonHm. So you're saying we should just be able to switch the two revision log fields in *ViewsData to use the generic 'field' plugin/handler, and it would be OK?
Comment #3
damiankloip CreditAttribution: damiankloip commentedYes, pretty much.
Comment #4
dawehnerSo we can mark this issue as fixed.
Comment #5
jhodgdonWell we still need to change those two fields to use the 'field' formatter and make sure it's working. I'll retitle and edit summary. And this is critical as per parent.
Comment #6
xjmComment #7
damiankloip CreditAttribution: damiankloip commentedOK then, so lets just remove that stuff..
Comment #8
damiankloip CreditAttribution: damiankloip commentedThat won't work, we still use that in aggregator! So I think it makes sense to just remove this in one issue if we're going to? Otherwise we will need another follow up after the aggregator issue just to remove the handler?
Comment #10
dawehnerFair, everything relies on twig for security anyway, so people using custom template engines are pretty much lost already, if you ask me.
Comment #12
damiankloip CreditAttribution: damiankloip commentedLet's break the dependency on aggregators Xss field handler, it doesn't really need the one we're removing at all. We can work out somewhere else if we really need to keep that one.
Comment #13
dawehner+1
Comment #14
alexpottIs this tested now?
Comment #15
damiankloip CreditAttribution: damiankloip commentedSame as before, tested briefly in Drupal\aggregator\Tests\Views\IntegrationTest.
Comment #16
jhodgdonThe docs in core/modules/aggregator/src/Plugin/views/field/Xss.php got garbled (indentation) in this patch. Also I don't understand why this patch is touching that plugin at all, since there's a separate issue for aggregator's use of XSS filters? Unless you want to close that one as a duplicate of this one and take care of both of them here?
I'm also confused as to why you are removing the "xss" filter from Views. This is a filter for generic non-entity database table data, and it seems like it could be useful to keep it around.
Comment #17
dawehnerSo to be clear, there is 'xss' as well as 'aggregator_xss' ... for the reason that there are fields which need a special xss checking.
This issue removes the XSS plugin itself, ... BUT the aggregator_xss plugin extended from it, so we had to adapt the aggregator_xss module to keep working.
This is something to discuss in general ... and we came to the conclusion that we rely more and more and auto-escaping, which means that views does not longer have to care about it.
Comment #18
jhodgdonI think that discussion about auto-escaping was about entity fields though... I am not sure the same would apply when reading raw information out of the database and using it in Views? A test for both would be more convincing: put something in a plain database field and/or an entity base field that needs to be escaped away or XSS filtered and verify that it is using the generic 'string' handler for Views or the 'field' handler for entities.
Comment #19
damiankloip CreditAttribution: damiankloip commentedViews escapes its own output anyway by default, this is done in the base plugin class, so I don't see a need to test functionality that has existed for a long time again. This will use check_plain by default. This has been around in views for...years. But we may not even need that in the long run.
So that is a change from just xss filtering before. Is that a change we can do? If not, we will need a mechanism so views data can declare the filtering it wants to use on the output.
New patch to fix the indentation.
Comment #20
damiankloip CreditAttribution: damiankloip commentedAnd yes, for reasons Daniel already explained we need to touch the aggregator Xss plugin, it's a simple change, and breaks a dependency we really don't need.
Comment #21
dawehnerRight, the XSS handle was always just a shortcut for developers for the underlying sanitizeValue method ... which is a feature which existed before autoescaping.
Comment #22
damiankloip CreditAttribution: damiankloip commentedYes, which after all of this, we could look at actually just removing altogether maybe...
Comment #23
alexpottCommitted 0884e7b and pushed to 8.0.x. Thanks!