Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
For example, adding a colon right after keyword+field pair completely brakes this keyword (the keyword+field and extra colon will be replaced with empty string).
Try this one (without quotes) as a title pattern for node_view
page: "%node:title: hello
". It will always be " hello
", but it shouldn't.
Patch follows.
Comment | File | Size | Author |
---|---|---|---|
#26 | ctools-1727804-26.patch | 1.17 KB | sylus |
#12 | 1727804-test-only.patch | 619 bytes | andypost |
#6 | ctools-1727804-6.patch | 1.69 KB | Dmitriy.trt |
#1 | ctools-1727804-1-test-only.patch | 2.84 KB | Dmitriy.trt |
#1 | ctools-1727804-1.patch | 3.45 KB | Dmitriy.trt |
Comments
Comment #1
Dmitriy.trt CreditAttribution: Dmitriy.trt commentedTest without the fix and with it.
Comment #2
merlinofchaos CreditAttribution: merlinofchaos commentedNice! It's not often someone provides tests!!
Comment #3
robcarrThis has caused downstream (untested) problems with tokens - see #1741064-1: Context token substitutions. Substitutions such as '%node:field-address:postal_code' (using the Address Field module) no longer work.
Not so sure this patch to this issue has really helped: having a ':' at the end of a token (eg,
%node:title: hello
) is plainly a syntax error made by a human, a little bit beyond an edge-case?Feel free to slap my wrists, by re-opening and effectively duplicating the issue...
Comment #4
Dmitriy.trt CreditAttribution: Dmitriy.trt commentedSorry, my mistake. This code needs a rewrite to support tokens with multiple levels and don't break on some edge cases. Unfortunately, don't have time to do it right now, but I'll find some time on this week.
@arrrgh Sometimes a colon right after the token is what you want, not a mistake. For example, you want to print "
Company title: product name
" using substitutions and you can't do it with previous regexp! There is no escape character or some other way to avoid breaking whole token by this colon.Comment #5
robcarrAnd here we enter Regex hell...
I'm tied up for next 2 weeks too, but did a quick 'undo' patch (as this has been committed to 7.x-1.2 release) at #1741064-2: Context token substitutions
Comment #6
Dmitriy.trt CreditAttribution: Dmitriy.trt commentedPatch with the fix of multi-level tokens broken by previous one. Also, adds test for this case.
Comment #7
robcarrAll looks fine with me - thanks for the quick rework @Dmitriy.trt. I daresay there are a few really extreme edge cases out there where the colon will screw things up... but I couldn't find them.
Considering that @merlinofchaos already committed the original change, and this was a minor additional tweak, I thinks it's safe to RTBC.
Comment #8
star-szrPatch in #6 works for me (using with Panels), RTBC +1.
Comment #9
andypost+1 RTBC all site:* & node:* tokens are broken!!! This really critical because leads to data loss because tokens are not availiable
Suppose better to use site:*token for test because node:* tokens could not be available (node module for D8 optional)
Comment #10
facal CreditAttribution: facal commentedPatch in #6 works for me as well, thank you!
RTBC +1
Comment #11
tim.plunkettThis should have simpletests.
Comment #12
andypostPatch with only fixed test from #6 - should fail
Comment #14
andypostCurrent functionality already covered with tests. #6 just extends this with multilevel token
Comment #15
robcarrAny idea if this patch will be committed? DEV snapshot on 25 Sep doesn't reflect the patch at #6.
What else needs doing?
Comment #16
andypostCan we get #6 commited?
Comment #17
goodeit CreditAttribution: goodeit commented#6: ctools-1727804-6.patch queued for re-testing.
Comment #18
goodeit CreditAttribution: goodeit commented#6 is working great for me, and it includes a test for the regression caused by #1. #1 provided tests for other situations.
So this has tests and is RTBC. Is it missing anything?
Comment #19
das-peter CreditAttribution: das-peter commentedI can confirm too, that #6 works as expected.
Comment #20
dillix CreditAttribution: dillix commentedI can confirm that #6 fixes this problem.
RTBC +1
Comment #21
devuo CreditAttribution: devuo commentedConfirm, the patch in #6 works as expected.
Comment #22
SpleshkaPatch in #6 is RTBC. Please, commit it ASAP.
Comment #23
robertom CreditAttribution: robertom commentedThanks for patch #6, it work like a charm.
Comment #24
SpleshkaAny progress in this issue?
Comment #25
oystercrackher CreditAttribution: oystercrackher commentedHas the patch in #6 been committed?
Please advise.
Thanks
Comment #26
sylus CreditAttribution: sylus commentedUpdating patch for latest dev sha: be26071
Comment #27
merlinofchaos CreditAttribution: merlinofchaos commentedDoh. And here I thought this had been fixed.
Committed and pushed. Thanks!
Comment #28.0
(not verified) CreditAttribution: commentedCorrect unexpected result