https://bugzilla.wikimedia.org/show_bug.cgi?id=69798
--- Comment #4 from Chris Steipp <[email protected]> --- Hi Nischay, Let me try to illustrate this with an examples. SpecialRecentActivityFeed.php: line 280 has $changeLine = $this->makeChangesLine( $list, $rc, $order ); if ( $changeLine !== false ) { $rclistOutput .= "<div style=\"margin-left:20px;\">$changeLine </div>"; } Looking at just this, it's hard to tell if this is safe, or what you intended to do for escaping. To show your escaping intent, a better way to write this would be, $rclistOutput .= Html::rawElement( 'div', array( 'style' => 'margin-left:20px;' ), $rclistOutput ); so it's clear to future developers that $rclistOutput is intended to be safe. Additionally, anyone wanting to add another div attributes later won't be tempted to put an unescaped value directly into the html. When reviewing this line, I'll wonder if $rclistOutput is correctly escaped, so to my last point, makeChangesLine should be commented with something like: @returns string An HTML snippet safe for output That way it's easy for me to check that function to see that it produces a safe html string, and later when someone edits the function, they know the expectation is that the function is handling the escaping. Does that make sense? > Most of the code here is borrowed (and restructured) from core MW classes > like ChangesListSpecialPage, SpecialRecentChanges and similar ones so if you > could provide examples from them it would be helpful. I haven't tracked down which pieces you brought in, but in general, if you can call or inherit from core functions/classes, that makes everything easier for long term maintenance. -- You are receiving this mail because: You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
