Posted by Dave Reid on January 11, 2010 at 9:57pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | token system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | pathauto |
Issue Summary
According to the token_replace() function documentation, the $options array parameter can include:
clear: A boolean flag indicating that tokens should be removed from the final text if no replacement value can be generated.
However, there is nothing that actually does this.
<?php
echo token_replace('mystring/[invalid:token]', array(), array('clean' => TRUE));
?>Expected:
mystring/Actual:
mystring/[invalid:token]
Comments
#1
#2
With tests this time.
#3
#2: 681782-token-clean-D7.patch queued for re-testing.
#4
Re-rolled for HEAD.
#5
Tagging core issues that will help pathauto in core/D7.
#6
Looks good and works fine on my local install, although for clarity the message returned by the two assertFalse's (with 'clear' and without) should probably be different and not both "Basic placeholder tokens replaced." Also, I know that this patch doesn't introduce this, but it's weird that the second assertFalse with "Basic placeholder tokens replaced." appears below that comment block talking about the sanitization tests.
#7
Sanitization is different from the 'clear'. The former is making sure there's no XSS in tokens, the latter is making sure we get rid of invalid, non-replaceable tokens. I'll revise the inline comments shortly.
#8
Revised in-line comments and test assertion messages. How's this one look mcarbone?
#9
The last submitted patch, 681782-token-clean-D7.patch, failed testing.
#10
#8: 681782-token-clean-D7.patch queued for re-testing.
#11
The last submitted patch, 681782-token-clean-D7.patch, failed testing.
#12
#13
Fixes look good to me but patch doesn't apply cleanly:
#14
Here, give this a try.
#15
Applies cleanly, works well, looks ready to go.
#16
#2: 681782-token-clean-D7.patch queued for re-testing.
#17
#1: 681782-token-clean-D7.patch queued for re-testing.
#18
Committed to CVS HEAD. Thanks.
#19
Automatically closed -- issue fixed for 2 weeks with no activity.