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.
Follow up from #2492839-116: Views replacement token bc layer allows for Twig template injection via arguments. Anytime you see the same code three times, it needs to be abstracted!
+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -357,34 +353,44 @@ protected function viewsTokenReplace($text, $tokens) {
assert('preg_match(\'/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/\', $token) === 1', 'Tokens need to be valid Twig variables.');
...
+ assert('preg_match(\'/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/\', $top) === 1', 'Tokens need to be valid Twig variables.');
...
+ assert('preg_match(\'/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/\', $key) === 1', 'Tokens need to be valid Twig variables.');
We should either provide this regex as a constant or provide a Twig::isValidVariable()
function.
Comment | File | Size | Author |
---|---|---|---|
#21 | 2567269-21.patch | 6.72 KB | rgoodine |
#11 | FireShot Screen Capture #069 - 'Подробности I SKITOURS' - skitours_com_ua_test_admin_reports_dblog_event_15842.png | 44.43 KB | fomenkoandrey |
#8 | 2567269-7-8.interdiff.txt | 1.76 KB | mikeker |
#8 | 2567269-8.Twig-utilities.patch | 5.96 KB | mikeker |
#7 | interdiff-4-7.txt | 680 bytes | snehi |
Comments
Comment #2
joelpittetThis seems like improve testing.
Comment #3
mikeker CreditAttribution: mikeker as a volunteer commentedHow about this as a starting point?
Comment #4
mikeker CreditAttribution: mikeker as a volunteer commentedComment #5
mikeker CreditAttribution: mikeker as a volunteer commentedSorry, meant to say in the previous comment:
Adds a few more tests and cleans up the incorrect naming of an existing test (the "...WithTokens" test was testing replacement without tokens).
Comment #6
joelpittet@mikeker thanks for pushing this along. Here's a review:
nit: extra space between @param and string.
Before the preg match was happening in a string in the assert. We discussed in another issue about performance and asserts are not called unless in dev and turned on. But this setup will call preg_match() and empty() and Html::escape(). Which will all have a negative performance impact. Can we get away without this?
nit: Missing a * on the next line and a period. Extra space between @param and string.
Comment #7
snehi CreditAttribution: snehi as a volunteer commentedAs mentioned in #6.
1. Fixed
2. Don't know which part to remove only preg_match or all the functions.
3. Fixed.
Comment #8
mikeker CreditAttribution: mikeker as a volunteer commented@joelpittet, Thanks for the review! From #6:
Good point.
If assert()
is passed a string, then it's not eval'ed if assertions are turned off. That will save us thepreg_match()
call in production. However, I don't think there is a way to avoidHtml::escape()
unless we know that the token keys are safe (I don't believe they are) or that assert messages are escaped before being displayed (I have no idea...). I suppose we could checkASSERT_ACTIVE
?Added a $this->fail() at the end of the try block. Otherwise the test never fails and that's not a very good test... :)
Comment #9
mikeker CreditAttribution: mikeker as a volunteer commentedAlso, this issue should always be tested against both PHP 5.5 and PHP 7 as the assert function changes substantially between the two versions.
Comment #11
fomenkoandrey CreditAttribution: fomenkoandrey commentedPHP7
drupal 8.1.3
Comment #12
mikeker CreditAttribution: mikeker as a volunteer commented@fomenkoandrey: Can you provide some more details about the view causing the assert. Specifically any tokens used in field rewrites.
Thanks.
Comment #13
fomenkoandrey CreditAttribution: fomenkoandrey commentedview: http://savepic.net/8212030.png
Field of date, when node was changed.
override output: {{ changed }}, {{ uid }}
http://savepic.net/8192574.png
Field body, cropped to 600 characters, then overriden to 300 and with link with token {{ path }}
http://savepic.net/8209982.png
http://savepic.net/8216126.png
http://savepic.net/8213054.png
3 filter:
author not admin http://savepic.net/8202814.png
changed <720 hours http://savepic.net/8203838.png
author - opened for users http://savepic.net/8207934.png
Comment #18
markhalliwellComment #20
rgoodine CreditAttribution: rgoodine as a volunteer commentedWorking on this for Global Sprint Weekend.
Comment #21
rgoodine CreditAttribution: rgoodine as a volunteer commentedAttached is the re-rolled patch.
Minor conflicts in `/core/modules/views/tests/src/Kernel/Plugin/PluginBaseTest.php` & `PluginBase.php` were resolved.
Comment #22
rgoodine CreditAttribution: rgoodine as a volunteer commented