Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
theme system
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
29 Aug 2015 at 01:24 UTC
Updated:
14 Sep 2015 at 07:54 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
star-szrHere's all the ones I can find.
Comment #3
star-szrAfter self-reviewing this one I realized this one needs another approach, but it might just be clearer to Html::escape() here. Would like a second opinion on it.
Comment #6
star-szrThere's definitely some things that are checking for a '#markup' key specifically. I haven't looked into the entity reference things yet.
This patch tries using Html::escape() in Drupal\rest\Plugin\views\display\RestExport, and also removes unused uses for SafeMarkup.
Comment #7
star-szrUpdating the ER tests, hopefully on the right track here.
Comment #9
alexpottThis use #plain_text and unset #markup.
Comment #10
stefan.r commentedThis implements #9, also looked for any further possible conversions and found another 2
Comment #11
alexpott@stefan.r I think the DiffFormatter changes will cause double escaping in the the config diff. The parent issue contains a test for this. I'd rather do the very simple and easy to follow changes here than make the changes to DiffFormatter without testing.
Comment #12
alexpottNeeds for #11
Comment #14
stefan.r commentedComment #16
stefan.r commentedLet's see if this fixes the test fail
Comment #17
dawehnerI'm wondering whether we should here think about whether using checkPlain or the corresponding #plain_text is the right thing to do for all those places ...
but I guess at least for now it is the most practical thing to just convert and rethink the usecases later ...
Comment #18
alexpott@dawehner yeah - this patch is functionally equivalent to HEAD - I think we should just do the conversions are be done.
Comment #19
alexpott#18 was also meant to RTBC this.
Comment #20
dawehner+1 for the RTBC
Comment #21
star-szrI'm not convinced that these test changes aren't hiding a regression.
Comment #22
alexpott@Cottser the problem with that test is that is it testing the render array rather than the output - we have tests that confirm #plain_text will always be escaped and we're testing here that #plain_text is set. Afaics there is no regression here.
Comment #23
alexpottThese are the lines that prove we don't have a regression.
Comment #24
star-szrYup, just came to that same conclusion myself. Back to RTBC, thanks :)
Comment #25
star-szrIt's mostly just that this assertion message is no longer accurate IMO, that's what initially threw me:
That would be true if we rendered the render array, but since we just check it directly it's not (yet) escaped.
Edit: But like you said we have separate tests that prove that. Just kinda makes this a weak integration test to me.
Comment #26
stefan.r commentedI had that same thought, technically we could render it if we're unsure #plain_text works as expected, I think we did something similar in the checkPlain() issue? @cottser feel free to update if you feel it makes the test stronger.
Comment #27
star-szrI could go either way on it, we can do it in a follow-up perhaps so as not to hold this up.
Comment #28
stefan.r commentedCool
Comment #29
catchI also think the straight conversion is OK here.
Committed/pushed to 8.0.x, thanks!