"Use context keywords" strips out anything after a % if it doesn't match

I've found this with URLs in HTML content that contain '%20' due to linking to files with spaces in the their names in the WYSIWYG editor.

Since I could not turn off keyword substitution, I patched ctools_context_keyword_substitute() in file ctools/includes/context.inc to ignore any keywords that started with '20', which preserves my '%20's.

It's not perfect, but I can't see many cases of genuine keywords starting with numbers, so it should be safe enough in my case.

This bug report has not been active for over six months. In an effort to clean-up the issue queue this item has been closed. If your modules are current and the report is still relevant please feel free to change the Status back to active.

Panels 7.x-3.2 works correctly

I ran into this on a site update, so in ctools "6.x-1.2" this was not a problem but in ctools "6.x-1.8" it is.
I looked at the function ctools_context_keyword_substitute() identified by matthine

it looks like previously if there was no matching context there was no replacement added to the $keywords array.
but now there is a replacement of empty string added.

On can simply choose not to replace the string at all if there is no value to replace it with instead of replacing it with and empty string. It seems like a valid choice either way, but with significantly different results.
To do this simply comment out the line $keywords['%' . $keyword] = ''; in ctools_context_keyword_substitute()

Or alternately but with much more overhead, check to see if the '%' . $keyword contains valid html entity and if so do not replace it.

Thoughts? Is there a compelling reason that you should replace the supposed keyword with and empty string?

This is primarily useful in things like titles and such when you have potentially empty keywords due to optional contexts. It is actually the intended behavior.

So it might be best if we check to see if it is, in fact, an html entity before deciding that we can't replace it. A simple check for a ';' as the last character of the keyword should suffice for that purpose.

Oops also my mistake in saying "html entity". Those & type values are not a problem, it's url encoded hex codes, %20 %2f %ca etc..
Due to the fact that one might have a keyword like %catalog I have not thought of a shortcut for identifying this case short of urldecode()

We can't assume that the whole found keyword is just one hex code since the text is likely to be something like this.
href="my%20file%20name.pdf"
so the keyword match found will be "%20file%20name"

On could make this check if (urldecode ( '%' . $keyword ) == '%' . $keyword)
but I don't know how expensive and extra urldecode is here, but that would solve the issue for my problem cases with a less drastic change in behavior.

OK so still and issue, I'm patching after an update.
But I thought of a couple alternate checks that I believe may be lighter weight.
Still not perfect but maybe enough to trigger someone else to think of a safe logical check?
I am of the opinion that it is a problem that I could not use context keywords in a text field that also includes a URL encoded string. Most end users when they encounter this are baffled.

in place of
$keywords['%' . $keyword] = '';
check

if (!is_numeric ( substr($context,0,1) )) {
  $keywords['%' . $keyword] = '';
}

or

if (!ctype_xdigit(substr($context,0,2))) {
  $keywords['%' . $keyword] = '';
}

The problem is the first above still fails on 20%-30% since - is not a number or hex.
The problem with the second is that some context like $data would match hex 'da'

If all you want to do is ignore potential keywords that start with numbers and/or hyphens, you can just change the regex that searches for keywords from this:

if (preg_match_all('/%(%|[a-zA-Z0-9_-]+(?:\:[a-zA-Z0-9_-]+)*)/us', $string, $matches)) {

to this:

if (preg_match_all('/%(%|[a-zA-Z_]+(?:\:[a-zA-Z0-9_-]+)*)/us', $string, $matches)) {

Works for me. I'm not aware of any situations in which actual keywords start with numbers or hyphens so this should do the trick.

Oops, should have read the patch first.

You should only assign an issue to yourself if you're going to work on it, otherwise please leave it unassigned. Thanks :)

This patch (#15) solves my problem, as we had the same issue that we had url encoded characters in a markup attribute, and that was stripped by this function without the patch.

Fair enough @codewatson, I agree. Feel free to provide a patch =D

Any news on this?

Patch #21 works for me!

Had an issue with menu blocks with link title enabled embedded into panels.

#Patch in #21 applies cleanly to current 7.x head.

code sniffer comes up with a couple of issues:

For:
if (isset($context_keywords[$keyword])) {

it suggests using array_key_exists(), and for:

$keywords['%' . $keyword] = ctools_context_convert_context(...

it notes that the function can return void (early on in the function is a "return;').

Not included in the changed lines, but in the same block, it notes that for:

if (preg_match_all('/%(%|[a-zA-Z0-9_-]+(?:\:[a-zA-Z0-9_-]+)*)/us', $string, $matches)) {

the 's' option on the regex is ambiguous, because there is no '.' in the pattern itself.

Overall, I don't think any of those issues are significant enough to defer applying the patch.

This patch was posted a year ago, and is RTBC since 4 months, I dont understand why its not committed yet.

@nagy.balint, it wasn't committed because nobody volunteered to get it committed. Offering your own time to help (what I am doing right now) is a good way to get things moving :-)

@rivimey, Well if an issue is rtbc then it should either be committed or it should be set back to needs work with a reason. But if there is no maintainer to look at rtbc issues then it does not matter what i do. It will never be committed.

Throwing a grand ole "Needs tests" on this because it's something that should be pretty easy to test.

Adding the tests Damien requested :-)

THIS PATCH ONLY ADDS TESTS: YOU STILL NEED #21 to fix the issue.
This is done so it's easy to do before/after patch testing.

Tested using
php ../../../../scripts/run-tests.sh --verbose --class CtoolsContextKeywordsSubstitutionTestCase

---- CtoolsContextKeywordsSubstitutionTestCase ----

Status Group Filename Line Function
--------------------------------------------------------------------------------
Pass Other context.test 13 CtoolsContextKeywordsSubstitutionTe
Enabled modules: ctools
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Multi-level token has been replaced. Colon left untouched.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Keyword and converter have been replaced.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Keyword after escaped percent sign left untouched.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Multiple substitutions have been replaced.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Colon after keyword and converter left untouched.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Escaped percent sign after keyword and converter left untouched.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Keyword after escaped and unescaped percent sign has been replaced.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Non-existant context ignored.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Non-keyword percent sign left untouched.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
HTTP URL escape left untouched.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
HTTP URL escape 2 left untouched.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Non-keyword percent sign left untouched.
Pass Other context.test 82 CtoolsContextKeywordsSubstitutionTe
Non-keyword percent sign left untouched.

This is the two patches (#21 and #34) in one file, to make it easier for japerry.

Lets get it into 1.12.

Thanks for 36. That passed, and looks good. Committed.

This commit caused a regression with panels titles.

New issue filed here:

https://www.drupal.org/node/2830559

#2830559: Regression: CTools Page Title %title in text (Use context keywords" strips out anything after a %)