Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

fix cache key usage #8695

Merged
merged 2 commits into from Apr 18, 2017
Merged

fix cache key usage #8695

merged 2 commits into from Apr 18, 2017

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Apr 8, 2017

This fixes #8694.

Explanation:

  • The Automator uses $objPages->dns ?: '*' here for creating the mappings of the empty request cache key.
  • However, FrontendTemplate::addToCache never actually uses * when creating the cache key (for the empty request), see here. Thus the cache key for the empty request will always be example.org/empty….
  • Thus FrontendIndex::outputFromCache never actually finds a cache entry for the empty request here since it will always be looking for */empty… due to the mapping (if no DNS setting was made in the root page).

Note: if the internal cache is not in use or created, then this Problem still remains.

@@ -251,7 +251,7 @@ protected function addToCache()
$strHeader = sprintf
(
"<?php /* %s */ \$expire = %d; \$content = %s; \$type = %s; \$files = %s; \$assets = %s; ?>\n",
$strCacheKey,
ltrim($strCacheKey, '*'),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to comment on why this is necessary: when the cache key starts with *, the resulting PHP header of the cache file would look like this:

<?php /* */empty.de */ $expire = 1494230344; $content = 'text/html'; $type = 'regular'; $files = ''; $assets = ''; ?>

This is invalid PHP syntax of course, thus I simply remove the * for the header.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "<?php\n\n// %s\n\$expire = %d;\n\$content = %s;\n\$type = %s;\n\$files = %s;\n\$assets = %s;\n\n?>\n" instead?

Copy link
Contributor Author

@fritzmg fritzmg Apr 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that would work too :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed it now.

@@ -213,7 +213,7 @@ protected function addToCache()
// If the request string is empty, use a special cache tag which considers the page language
if (\Environment::get('request') == '' || \Environment::get('request') == 'index.php')
{
$strCacheKey = \Environment::get('host') . '/empty.' . $objPage->language;
$strCacheKey = ($objPage->domain ?: '*') . '/empty.' . $objPage->language;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be $objPage->dns?

Copy link
Contributor Author

@fritzmg fritzmg Apr 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, $objPage->dns is only available for root pages. For other pages this information is available in ->domain. See system/modules/core/models/PageModel.php#L868

@@ -251,7 +251,7 @@ protected function addToCache()
$strHeader = sprintf
(
"<?php /* %s */ \$expire = %d; \$content = %s; \$type = %s; \$files = %s; \$assets = %s; ?>\n",
$strCacheKey,
ltrim($strCacheKey, '*'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "<?php\n\n// %s\n\$expire = %d;\n\$content = %s;\n\$type = %s;\n\$files = %s;\n\$assets = %s;\n\n?>\n" instead?

@leofeyer leofeyer self-assigned this Apr 18, 2017
@leofeyer leofeyer changed the base branch from master to hotfix/3.5.26 April 18, 2017 12:01
@leofeyer leofeyer merged commit 7594240 into contao:hotfix/3.5.26 Apr 18, 2017
leofeyer pushed a commit that referenced this pull request Apr 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants