Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.$attrbutes always has a space on when it calls Xss::attributes which leads to unnecessary calls to preg_match() and preg_replace().
Measurements
On the user permissions page with 11 roles:
| Function Name | Calls Diff | Calls Diff% |
Incl. Wall Diff (microsec) |
IWall Diff% |
Excl. Wall Diff (microsec) |
EWall Diff% |
Incl. MemUse Diff (bytes) |
IMemUse Diff% |
Excl. MemUse Diff (bytes) |
EMemUse Diff% |
Incl. PeakMemUse Diff (bytes) |
IPeakMemUse Diff% |
Excl. PeakMemUse Diff (bytes) |
EPeakMemUse Diff% |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| trim | 751 | 51.9% | -10 | -0.0% | -10 | -0.0% | 30,624 | 2036.2% | 30,624 | 2036.2% | 0 | 0.0% | 0 | 0.0% |
| strlen | -733 | -50.6% | -15 | -0.0% | -15 | -0.0% | -16 | -1.1% | -16 | -1.1% | 0 | 0.0% | 0 | 0.0% |
| preg_match | -733 | -50.6% | -695 | -0.7% | -695 | -0.7% | -256 | -17.0% | -256 | -17.0% | -1,640 | -101.5% | -1,640 | -101.5% |
| preg_replace | -733 | -50.6% | -3,201 | -3.4% | -3,201 | -3.4% | -113,632 | -7555.3% | -113,632 | -7555.3% | -1,472 | -91.1% | -1,472 | -91.1% |
Comments
Comment #1
alexpottHere's a patch.
Comment #2
catchNice find!
Comment #5
alexpottThe first test run failed because it got terminated on a long running bot.
Comment #6
dawehnerReplacing two regular expressions with a single trim doesn't sound like the worst of an idea :)
Comment #7
alexpottThis exists in D7 too.
Comment #8
Wim LeersWow, impressive find :)
Comment #9
xjmSo I made the mistake of trying to understand why this was necessary by reading Xss::split() and Xss::attributes(). Save yourself.
I did however find myself wondering why the regex doesn't just exclude the optional leading and/or trailing space from the match? @alexpott is checking this out.
Comment #10
alexpottI thought we needed to worry about space on the front and on the end of the
$attrlistvariable inXss::split(). I was wrong. The regexes inXss::attributes()are greedy when it comes to end space. So we can lose the trim. Nice call @xjmTest scenario: standard profile, user/permissions, and no additional roles added - so just the 3 that come with core.
I don;t think wall time is indicative of anything.
Diff%
Obviously there is the possibility the additionally whitespace matching in the regex is more costly than the trim() but that'd surprise me.
Comment #11
Wim LeersComment #12
catchWould be good to see the profiling comparison just to see that loop still gone.
Comment #13
alexpott@catch that's in #10 - no calls to trim now and just less function calls.
Comment #15
catchSorry missed that somehow.
Committed/pushed to 8.0.x, thanks!
Comment #16
alexpottThis additional loop happens in D7 too.
Comment #17
cilefen CreditAttribution: cilefen as a volunteer commentedComment #23
joseph.olstadRTBC, I just checked D8, the same code has been there since it was committed two years ago and is still in 8.5.x, if it's good enough for D8, it's good enough for D7. Passes all tests. I cannot find any logical reason why this should not be committed. If D8 is production ready, then this patch should be ready for D7. So therefore saying this patch not ready for D7 means that D8 is not ready for production. I doubt that anyone is willing to say otherwise.
Patch #17 still applies albeit with offset, reroll same.
Comment #24
joseph.olstadall green , kicked php 5.4 again, it passes, all currently supported versions pass tests. This is a win!
Comment #25
PolThis is fine for me as well.
Comment #26
PolComment #27
joseph.olstadThis was fixed in 8.0.x
marking this as fixed
As per the backport policy, I created a new D7 issue , for the D7 backport of the same, see:
#3008166: [D7] Unnecessary looping in Xss::filter when processing attributes.
Comment #28
joseph.olstadremoved tags , they're present on the D7 backport issue