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

Hi Sylus, thanks for your report.

I am not sure what to make of it though: the context.test file includes a number of tests for various replacement patterns and as far as I know 1.12 passes them. It leaves me wondering whether this might be a case of "because of a bug/misfeature the earlier behaviour happened to work before", but have no evidence for that either :-)

Can you elaborate on your panels/ctools configuration, especially on the specific pane involved here? Ideally, a working 'Feature' demonstrating it would be great.

Alternatively, would you be able to check whether the context variable named 'title' does indeed exist for that pane at that time?

I'm seeing this issue on a client site here: http://foodpsychology.cornell.edu/discoveries

I was able to replicate the issue on a clean Drupal 7.53 install with just panels, ctools, page_manager, and features enabled. Attached is a feature module that creates a page at /issue_2830559 with the page title in the left region.

I also attached a screenshot of what I'm seeing.

Thank you @sylus for tracking this down.

I'm not suggesting that this patch be applied to the project, but I'm adding it here so I can quickly use it on a client project via https://bitbucket.org/davereid/drush-patchfile

As per #4 needs more information, postponing until supplied.

We also need to improve the test coverage.

I think the code supplied in #5 should be a reasonable starting point to work from.

Bumping to critical and adding the ctools 1.13 release.

I encountered this bug today in a situation where ctools_context_keyword_substitute() received '%title' as its string argument and array('%title' => '') as its $keywords argument. Before 1.12, this situation would return ''. After 1.12 it returns '%title'.

My patch addresses this situation on line 659. Instead of breaking the match processing loop only if $keywords['%title'] is not empty, it now breaks the loop if $keywords['%title'] is set. This seems in keeping with the spirit of the comment that reads "If the keyword is already set by something passed in, don't try to overwrite it," since in my case the keyword was already set to something. It just happened to be set to something that PHP interprets as empty. But I've never dug into this part of ctools before, so I can't say with confidence that this solution is in keeping with the intent of the code.

One reason this was not caught through automated testing is that the testKeywordsSubstitution() call in context.test passed an empty array as the $keywords variable into ctools_context_keyword_substitute() for every test. I refactored the test so that every test case gets my array('%title' => '') $keywords argument and added a test that validates the return value in my particular case. All of these tests now pass, but since ctools_context_keyword_substitute() generates a return value through the interaction of the $keywords argument with other arguments I'd say that the test coverage still needs expansion.

Thanks for your efforts. I think the 'isset' change is fine, just a couple of points on the tests themselves:

1. +++ b/tests/context.test
   @@ -36,6 +36,11 @@ class CtoolsContextKeywordsSubstitutionTestCase extends DrupalWebTestCase {
   +    // Input some values for the $keywords array that might cause problems.
   

   Suggest changing 'Input' to 'Some test'

2. +++ b/tests/context.test
   @@ -36,6 +36,11 @@ class CtoolsContextKeywordsSubstitutionTestCase extends DrupalWebTestCase {
   +      '%title' => '',
   

   Suggest adding more test cases. How about NULL, an integer, and perhaps a value containing a substring '%title' ?

Had to refactor the test a bit. Since we're now passing the same $string in for different tests, we can't use it as the key of the $checks array any more.

'%title' => NULL failed with my earlier fix, so I've changed isset() to array_key_exists().

Thanks, that looks fine now.

Committed.