3076: ding webtrekk fixes #928
Conversation
|
Regarding the "OSS parameters was added twice": I see there was added some checks to prevent this, but the problem was that OSS tracking for node search was added in search ting page also, because of changes in https://platform.dandigbib.org/issues/629. |
| @@ -80,8 +80,7 @@ function ding_webtrekk_panels_pane_content_alter($content, $pane, $args, $contex | |||
| * Track internal search results. | |||
| */ | |||
| function ding_webtrekk_views_post_execute(&$view) { | |||
| $count = $view->total_rows; | |||
| if ($view->name == 'ding_multiple_search' && $count > 0) { | |||
| if ($view->name == 'ding_multiple_search' && strpos(current_path(), 'search/node') === 0) { | |||
There was a problem hiding this comment.
Why do we need the current path check here?
There was a problem hiding this comment.
Because else it's added on ting search page also because of https://platform.dandigbib.org/issues/629
There was a problem hiding this comment.
OK. Then I would appreciate a comment explaining this.
There was a problem hiding this comment.
Oh yeah sorry, of course. Coming up :)
| @@ -62,10 +62,10 @@ function ding_webtrekk_page_alter(&$page) { | |||
| * Track ting search results. | |||
| */ | |||
| function ding_webtrekk_panels_pane_content_alter($content, $pane, $args, $contexts) { | |||
| if ($pane->type == 'search_result' && $pane->configuration['type'] == 'ting_search') { | |||
| if ($pane->type == 'search_result' && $pane->configuration['type'] == 'ting_search' && strpos(current_path(), 'search/ting') === 0) { | |||
There was a problem hiding this comment.
The combination of the implemented hook and the dependency on the current path does not seem right. The reliance on panes seems to be unnecessary noise.
How about using another hook instead and just checking whether drupal_static('ting_search_results', FALSE) is set? Perhaps hook_page_alter is really the way to go. Another approach could be hook_search_page
There was a problem hiding this comment.
I don't know, this was the way it was done in the old module. Just wanted to ensure that it only gets added on the correct page.
There was a problem hiding this comment.
If I remember correctly it was done this way because of:
- the search_result pane can be configures to use other "search types".
- drupal_static('ting_search_results', FALSE) might be set in other pages than the ting_search page.
- The current path check is there to aviod it being added on other pages, where the search wasn't triggered by a user.
There was a problem hiding this comment.
The static variable drupal_static('ting_search_results', FALSE) is set in ting_search_search_execute. So maybe it's safe to check for it in hook_page_alter.
Should I make this update and remove hook_panels_pane_content_alter() all together?
Just have the feeling that I forgotten something and it was made this way for a reason (made it myself, but can't remember tbh)
There was a problem hiding this comment.
Looked into hook_search_page. It seems it's only invoked on the module implementing the search type?
There was a problem hiding this comment.
@holt83: Then I think we should switch to hook_page_alter and check current path. hook_panels_pane_content_alter() can be removed entirely.
There was a problem hiding this comment.
Ok. I will update PR with this approach
|
@kasperg I updated the PR with the suggested changes. I tried my best to make helpful comments. Let me know if they don't make sense or if you have suggestions for improvements :) |
holt83
force-pushed
the
3076-ding-webtrekk-fixes
branch
from
8574d89
to
f761cde
Compare
|
Approved - but branched from wrong |
https://platform.dandigbib.org/issues/3076
OSS parameters was added twice because of the changes in https://platform.dandigbib.org/issues/629.
Updated the page titles also. Before the page title for Search node, was ding_multiple_search.
Also includes the fix to track empty searches.