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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dmitriy.trt’s picture

Status: Active » Needs review
FileSize
3.45 KB
2.84 KB

Test without the fix and with it.

merlinofchaos’s picture

Status: Needs review » Fixed

Nice! It's not often someone provides tests!!

robcarr’s picture

Status: Fixed » Needs work

This 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...

Dmitriy.trt’s picture

Sorry, 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.

robcarr’s picture

And 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

Dmitriy.trt’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

Patch with the fix of multi-level tokens broken by previous one. Also, adds test for this case.

robcarr’s picture

Status: Needs review » Reviewed & tested by the community

All 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.

star-szr’s picture

Patch in #6 works for me (using with Panels), RTBC +1.

andypost’s picture

Priority: Normal » Critical

+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)

facal’s picture

Patch in #6 works for me as well, thank you!
RTBC +1

tim.plunkett’s picture

Priority: Critical » Major
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This should have simpletests.

andypost’s picture

Status: Needs work » Needs review
FileSize
619 bytes

Patch with only fixed test from #6 - should fail

The last submitted patch, 1727804-test-only.patch, failed testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Current functionality already covered with tests. #6 just extends this with multilevel token

robcarr’s picture

Any idea if this patch will be committed? DEV snapshot on 25 Sep doesn't reflect the patch at #6.

What else needs doing?

andypost’s picture

Can we get #6 commited?

goodeit’s picture

#6: ctools-1727804-6.patch queued for re-testing.

goodeit’s picture

#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?

das-peter’s picture

I can confirm too, that #6 works as expected.

dillix’s picture

I can confirm that #6 fixes this problem.
RTBC +1

devuo’s picture

Confirm, the patch in #6 works as expected.

Spleshka’s picture

Patch in #6 is RTBC. Please, commit it ASAP.

robertom’s picture

Thanks for patch #6, it work like a charm.

Spleshka’s picture

Any progress in this issue?

oystercrackher’s picture

Has the patch in #6 been committed?

Please advise.

Thanks

sylus’s picture

FileSize
1.17 KB

Updating patch for latest dev sha: be26071

merlinofchaos’s picture

Status: Reviewed & tested by the community » Fixed

Doh. And here I thought this had been fixed.

Committed and pushed. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Correct unexpected result