When we pass data objects for token_replace function it is better to have send it keyed with name of the token.

What I mean is CurrentSearchItemText::execute() prepare $data as array('facetapi_results' => array('facetapi_adapter' => $adapter)), instead of array('facetapi_adapter' => $adapter) as we are going to replace tokens from facetapi_results group and not facetapi_adapter.

This also comes critical if we want to reuse tokens provided by facetapi. In my case it is issuse #1406364: Integration with Page Title module.

#1 facetapi-1406606-proper-key-for-tokens.patch1.33 KBygerasimov
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]


ygerasimov’s picture

Status:Active» Needs review
new1.33 KB
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]

Here is patch.

cpliakas’s picture

Status:Needs review» Needs work

Hi ygerasimov.

Thanks for bringing this up. I am trying to wrap my head around why it is critical that the data be named the same as the Token type. It looks like all other implementations of hook_token_info place the object in a top level key of the array, however they do usually match the type like you mentioned. To me it would seem weird to nest the adapter under a "facetapi_results" key, because I am not sure what it really accomplished. The adapter is really the data that needs to be required. I am definitely open for discussion on this, I just want to make sure their is a clear and critical reason to make the switch.


cpliakas’s picture

Status:Needs work» Closed (won't fix)

Hasn't been a lot of activity here, so marking as won't fix. Feel free to re-open if there is a driving need to implement this change.

DYdave’s picture

Version:7.x-2.x-dev» 7.x-1.x-dev
Status:Closed (won't fix)» Needs review

Hi guys,

Getting back to this ticket, since we've been brought to using a few of the facetapi tokens lately.
It seems indeed that the facetapi_results tokens don't really work as expected.

Pretty much, without applying this patch, it seems the [facetapi_results] tokens are simply not replaced anywhere.
It seems that without this patch the tokens: [facetapi_results] are all useless, since they don't get replaced.

After applying the patch to facetapi-7.x-1.2+8-dev (2012-10-18) it seems indeed that the [facetapi_results] tokens are properly replaced, which fixes this issue and makes these tokens useful again.

I certainly understand cpliakas' concerns from note#2, but didn't investigate any further than that, in particular, I'm not sure whether the top level keys would have to be modified in the array or not. But one thing I'm pretty sure about at the moment is that these tokens don't get replaced as expected.

Therefore, I was wondering whether the patch could be reviewed/modified and perhaps committed to module's code.

I would greatly appreciate any feedback on this.
Please let me know if you would need more information on the tests or this issue.

Thanks in advance.

cpliakas’s picture

I probably just dropped the ball on the review and didn't fully evaluate. Anyways, you are not alone as someone reported this at #1812196: facetapi_results tokens aren't replaced.

cpliakas’s picture

Status:Needs review» Closed (duplicate)

Re-posted patch to the bug report, closing this as a dup.

cpliakas’s picture

Patch in #1 has been committed to Facet API at #1812196-11: facetapi_results tokens aren't replaced.