Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
_update_message_text calls SafeMarkup::set() which is meant to be for internal use only.
Beta phase evaluation
Issue category | Task because not a bug, |
---|---|
Issue priority | Major because part of critical meta #2280965: [meta] Remove or document every SafeMarkup::set() call |
Proposed resolution
- Remove the call by refactoring the code.
Remaining tasks
- (done)Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
- (N/A Since this is not a bug (no double escaping) I think we do not need to add tests.) Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
Manual testing steps (for XSS and double escaping)
N/A
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#59 | Screen Shot 2015-08-12 at 5.28.40 PM.png | 195.14 KB | josephdpurcell |
#58 | interdiff-2501683-55-58.txt | 1.38 KB | josephdpurcell |
#58 | 2501683-58.patch | 4.24 KB | josephdpurcell |
#57 | Screen Shot 2015-08-12 at 5.00.17 PM.png | 211.42 KB | josephdpurcell |
#55 | 2501683-55.patch | 4.21 KB | stefan.r |
Comments
Comment #1
sclapp CreditAttribution: sclapp as a volunteer commentedComment #2
sclapp CreditAttribution: sclapp as a volunteer commentedComment #3
sclapp CreditAttribution: sclapp as a volunteer commentedRemoved string concatenation & used 2-part string with space separator and passed that through SafeMarkup::format().
Comment #4
sclapp CreditAttribution: sclapp as a volunteer commentedComment #6
joelpittetPlease move this to your global .gitignore.
Otherwise I think this is RTBC.
Comment #7
sclapp CreditAttribution: sclapp as a volunteer commentedincorporated feedback from #6
Comment #8
joelpittetShort array syntax for the end so we don't have mixed syntax in the same line.
Same here.
"Anything in $text has been run through t() and marked as safe."
Comment #9
sclapp CreditAttribution: sclapp as a volunteer commentedfixed up syntax to be consistent for arrays & fixed up comment to be a little clearer
Comment #10
joelpittetWhoops sorry, these array syntax changes are out of scope of this patch because these lines weren't needing change. The array syntax change should only change on the lines you were working on. Sorry if I miss lead you on that instruction.
The reason is someone in another patch may be working on those and if this patch got committed they would have to re-roll their patch. Or that's the theory.
All the other changes are great and are RTBC.
Comment #11
sclapp CreditAttribution: sclapp as a volunteer commentedhopefully, just removed the out of scope changes to syntax while including earlier patch code
Comment #12
joelpittetI should be a bit more thorough:( Sorry for keep sending this back.
The $linktext should separate the words with an underscore. like $link_text. Maybe there is a better variable name for this? $additional_text? Or $available_updates_text?
These need indenting back.
These lines are not needed.
These two lines should be indented on the same line. (so back 2 spaces for the new one)
Comment #13
kgoel CreditAttribution: kgoel at Forum One commented1. I have changed the variable from $linktext to $available_updates_text.
2. fixed
3. fixed
4. fixed
I have not changed the array syntax to make it short array syntax but made it consistent with other array in the _update_message_text function.
Comment #14
YesCT CreditAttribution: YesCT commentedthanks for updating that.
yeah, so we can use git diff --color-words, I think it is ok to not change the array syntax.
missed one of the alignments.
Comment #15
kgoel CreditAttribution: kgoel at Forum One commentedThank you for review!
It's so embarrassing that I missed one of the indentation :(
Comment #16
YesCT CreditAttribution: YesCT commentedI'm not sure which lines @joelpittet said where not needed in #12 3.
Comment #17
joelpittetSo what I was trying to say was that everything that remains in this function at the end is to be returned so the else itself is not needed.
Just make the hunk read:
Without the else.
Comment #18
kgoel CreditAttribution: kgoel at Forum One commentedComment #19
YesCT CreditAttribution: YesCT commentedfrom the issue summary template (for safemarkup issues)
Since this is not a bug (no double escaping) I think we do not need to add tests.
Manually tested, looking at
markup
or
are both the same in head and with the patch.
screenshot also shows this still looks ok (the same)
Comment #20
YesCT CreditAttribution: YesCT commentedComment #21
kgoel CreditAttribution: kgoel at Forum One commentedAdded beta evaluations
Comment #22
xjmAssigning to myself for additional feedback, but leaving at RTBC for now.
Comment #24
davidhernandezThis was RTBC so I assume it just needs a reroll?
Comment #25
izus CreditAttribution: izus commentedAnd here is the reroll
Comment #26
lauriiiEverything seems to work on my manual tests. Settings this back to RTBC for xjm's review.
Comment #27
xjmDiscussed with @alexpott; he has a suggestion for this one.
Comment #28
alexpottIt's a shame that
_update_message_text
can't return a render array because it is being used in hook_requirements as this would allow us to pass back[#markup => $text]
and then renderPlain in the email use case.That said we do have #2505499: Explicitly support render arrays in hook_requirements() open.
Comment #29
xjmNR to see if we can implement part of #28?
Comment #30
dawehnerWhat about getting #25 in and adding a todo to #2505499: Explicitly support render arrays in hook_requirements() ?
Comment #31
lauriiiI don't think #2505499: Explicitly support render arrays in hook_requirements() is necessary in order to get this fixed. That issue also could be worked on later to get it in for 8.1.x so +1 for @dawehners suggestions.
Comment #32
YesCT CreditAttribution: YesCT commentedI dont think we are waiting on anything else from @alexpott here. and we have somethings someone could try and do. so unassigning.
Comment #34
kgoel CreditAttribution: kgoel at Forum One commentedPatch from #25 still applies. Anyone up for review the patch and help move this issue forward?
Comment #35
YesCT CreditAttribution: YesCT commentedI'm going to work on this now.
Comment #36
YesCT CreditAttribution: YesCT commentedadded the @todo.
[edit. oops. wrong patch file. new one coming]
Comment #37
YesCT CreditAttribution: YesCT commentedre-verified that the markup on /admin/reports/status is still fine.
Comment #38
YesCT CreditAttribution: YesCT commentedinterdiff from 25.36 was good, but here is a patch with all the changes.
Comment #39
YesCT CreditAttribution: YesCT commentedupdating the reason for no test changes.
Comment #40
lauriiiCode looks good and visually it has remained same so this is RTBC for me. Making hook_requirements support render arrays is nice thing to do but doesn't fit the scope of this issue so its nice it gets done in follow-up. We possibly can even do that without BC break.
Comment #41
xjmDiscussed with @alexpott -- I'm going to tweak the documentation on this.
Comment #42
xjmActually, in the process of tweaking this, I finally remembered what @alexpott and I discussed a month or two back (and never wrote down, doh!).
As stated in the documentation for _update_message_text(), the output is used in two places: in the Update module's
hook_requirements()
implementation, and in itshook_mail()
implementation. Here are those two usages.In _update_requirement_check() which in turn is used only in
update_mail()
:Then in update_mail():
Note that in the first case, the
$report_link
parameter is always true and the$langcode
parameter is always empty, whereas in the second case,$report_link
is always false. Now look at this hunk of code at the end of_update_message_text()
:If
$report_link
is true,$langcode
and therefore$language
will always be null given the usages above! So the use of both here doesn't make any sense. Furthermore, since the link to the available updates page is only used in the helper function forupdate_requirements()
, there's no reason to have the unimplemented, untested code path for it forupdate_mail()
.Therefore, the attached patch adds the report link in
_update_requirement_check()
instead, removes the unneeded language checking for that link, removes the need for the extra boolean flag parameter altogether, and makes_update_message_text()
return a simple single translated string based on the switch logic.Hopefully I haven't missed anything. I also personally think we should deal with the
hook_requirements()
render array thing since it's only a small bit of documentation, but let's make sure this is sane first.I think the signature and return value change for
_update_message_text()
is appropriate and does not require a CR or anything because it is prefixed by a_
(thus "internal") and we're essentially removing a dead code path that unnecessarily complicates the assembly of this string.Comment #43
xjmNit: This line of code does nothing. ;)
Comment #45
xjmAlso another silly mistake.
Comment #46
xjmManually tested using the testing in #19 (thanks @YesCT) and confirmed it still looks the same. Aside: an out-of-scope usability/accessibility bug is that we have the words "available updates" linking to two different pages in the same paragraph...! You can see this in the screenshot in #19.
Here is the interdiff that, applied with #43, simply uses a render array instead. Note that this already works--we already support render arrays for
hook_requirements()
. It's just not documented as such. I tested manually in exactly the same way and confirmed this.So, my preference would be to just use that now, without blocking it on #2505499: Explicitly support render arrays in hook_requirements() which should just be a docs patch.
Comment #47
xjmFor the link text.
Comment #48
YesCT CreditAttribution: YesCT commentedI'm going to review this now, and take care of the follow-up.
Comment #49
YesCT CreditAttribution: YesCT commentedso @xjm found out #2505499: Explicitly support render arrays in hook_requirements() is actually postponed and wont just be a docs patch.
Comment #50
YesCT CreditAttribution: YesCT commented#2545520: The same link "available updates" links to two different pages in the same paragraph when there is a problem checking available updates opened.
and I didn't get more though the review, so that is up for grabs now. :) Done for now.
Comment #51
joelpittetReviewed all changes and code-paths in this change to ensure anything wasn't missed. +1 it's nice little clean-up even with the @todo regarding SafeMarkup::format vs render array.
There is a double space after the
=
that could be cleaned up on commit.Comment #52
stefan.r CreditAttribution: stefan.r commentedre #49, looks like this is going ahead after all (see #2505499: Explicitly support render arrays in hook_requirements())
So should this use a render array after all?
Comment #53
alexpottComment #54
xjmYep, let's update this to be a render array. Thanks @stefan.r!
Comment #55
stefan.r CreditAttribution: stefan.r commentedsee #19
Comment #56
josephdpurcell CreditAttribution: josephdpurcell commentedReviewing and doing manual testing.
Comment #57
josephdpurcell CreditAttribution: josephdpurcell commentedInitial manual test shows there is no space between sentences, see screenshot:
Comment #58
josephdpurcell CreditAttribution: josephdpurcell commentedThese spaces were removed causing the issue I noticed where there is no space between sentences.
This patch will fix it by adding those spaces back in as a #prefix. Use #prefix to avoid unnecessarily needing to escape and filter that space.
Comment #59
josephdpurcell CreditAttribution: josephdpurcell commentedManually checking this, here is the screenshot:
Also, I removed the screenshot from the description since I assume that was put there by accident.
Comment #60
dsnopekI manually tested @josephdpurcell's patch from #58. There was no change in the message on the "Status report" page from current core (so it matches the screenshot from #58 and has the space between the two sentences).
Since this was RTBC before it was marked as "Needs manual testing" (in #51), I'm setting back to RTBC!
If the testbot agrees, this might be ready. :-)
Comment #61
alexpottShould we still wait for #2505499: Explicitly support render arrays in hook_requirements()? Maybe as this code only fires in the
runtime
requirements checks maybe it's ok to go now?Comment #62
xjm@alexpott I don't think it needs to wait since it's the runtime phase only.
Comment #63
xjmComment #64
alexpott@xjm I agree.
Committed 8ba635b and pushed to 8.0.x. Thanks!