Problem/Motivation
I'm not 100% sure but I think I found a bug in the current caching implementation of the _etracker_XXX_should_be_tracked implementations.
While the "parameter" like path, role or user given to the method is implicit (determined in the function), the caching will always return the same value from drupal_static() even if the user or path is not the same.
I guess this is just theoretical and minor therefore, as I think drupal_static() is typically "cleared" for each new request(?) but there may be caching modules or mechanisms which persist the values over requests and in that case, the results may definitely become wrong!
See this code:
/**
* Tracking visibility check for pages.
*
* Based on visibility setting this function returns TRUE if JS code should
* be added to the current page and otherwise FALSE.
*
* @param \Drupal\Core\Config\ImmutableConfig $config
*
* @return array|bool
*/
function _etracker_path_should_be_tracked(ImmutableConfig $config) {
$path_should_be_tracked = &drupal_static(__FUNCTION__);
// Cache tracking result if function is called more than once.
if (!isset($path_should_be_tracked)) {
$visible_path_mode = $config->get('etracker_track_path_mode');
$visible_path_pages = $config->get('etracker_track_paths');
// Match path if necessary.
if (!empty($visible_path_pages)) {
// Convert path to lowercase. This allows comparison of the same path
// with different case. Ex: /Page, /page, /PAGE.
$pages = mb_strtolower($visible_path_pages);
$path = Drupal::service('path.current')->getPath();
$path_alias = \Drupal::service('path_alias.manager')->getAliasByPath($path);
if (empty($path_alias)) {
$path_alias = mb_strtolower($path);
}
else {
$path_alias = mb_strtolower($path_alias);
}
$page_match = \Drupal::service('path.matcher')->matchPath($path_alias, $pages) || (($path != $path_alias) && \Drupal::service('path.matcher')->matchPath($path, $pages));
// When $visible_path_mode has a value of 'all_pages', the tracking
// code is displayed on all pages except those listed in $pages. When set
// to 'all_listed', it is displayed only on those pages listed in $pages.
$track_all_paths = ($visible_path_mode == Constants::ETRACKER_TRACK_PATHS_MODE_ALL);
$path_should_be_tracked = ($track_all_paths xor $page_match);
}
else {
$path_should_be_tracked = TRUE;
}
}
return $path_should_be_tracked;
}
with more explicit dependencies (as function parameters) the $path would be a parameter passed to the function. Currently it's fetched in the function but that doesn't mean it can't change, as it's still an external value.
But due to
&drupal_static(__FUNCTION__);
drupal_static will always return the first result, even if path is no more the same at second call. Typically the functions result SHOULD BE different for a different path and only be cached for that path, see https://drupal.psu.edu/blog/post/implementing-drupalstatic-function-dyna....
So correct this line should be:
&drupal_static(__FUNCTION__ . $path);
with the $path given from the calling function.
See https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...
Sidenote:
Keep in mind that it might still make sense to nest the conditions, for example only fetch the user or path, if the key is set and the other function didn't return false. For performance reasons.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes