-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
REQUEST_URI in default context #123
Comments
Does not seem like correct. I'll be looking into this. |
More information: Setting EXPOSE_SERVER = true fixes the problem. @sanmai @asacarter should we have a whitelist instead of a blanket ban on $_SERVER variables? |
Here's a quick workaround filter that patches the problem: $template->registerFilter('patch_pagination_url', function ($pagination_url) {
if (filter_var($pagination_url, FILTER_VALIDATE_URL)) {
return $pagination_url;
}
// Broken URL due to missing REQUEST_URI in context
// e.g. http://?page=2
// Naive fix assuming that there's exactly one query param
$page = explode('=', $pagination_url)[1];
$url = (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'on' ? "https" : "http")
. "://$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$url .= (parse_url($url, PHP_URL_QUERY) ? '&' : '?')
. \Liquid\Liquid::get('PAGINATION_CONTEXT_KEY') . '=' . $page;
return $url;
}); Usage: {{ paginate products by 6 }}
Next URL: {{ paginate.next.url | patch_pagination_url }}
{{ endpaginate }} |
I have nothing against an allow-list of sorts. Care to make a PR? |
Since updating the library, REQUEST_URI is no longer available in the default context but is referred to within the Pagination tag.
Is this the correct or should it be set manually?
The text was updated successfully, but these errors were encountered: