Skip to content
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

Add caching for theme yaml file #8202

Merged
merged 1 commit into from Aug 10, 2017

Conversation

jocel1
Copy link
Contributor

@jocel1 jocel1 commented Jul 31, 2017

Questions Answers
Branch? develop
Description? Add caching for theme yaml file to speed up load time
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
How to test? Load a product page

@jocel1
Copy link
Contributor Author

jocel1 commented Jul 31, 2017

@jocel1 jocel1 changed the title BO: Add caching for theme yaml file Add caching for theme yaml file Jul 31, 2017
}

if ($this->filesystem->exists($jsonConf)) {
$data = $this->getConfigFromFile($jsonConf);
} else {
$data = $this->getConfigFromFile($dir.'/config/theme.yml');
file_put_contents($jsonConf, json_encode($data));
Copy link
Member

Choose a reason for hiding this comment

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

Can you please use the dumpFile() function from the class Filesystem?
The instance is available in $this->filesystem.

@Quetzacoalt91
Copy link
Member

So you're telling me the cache was never created, although we always checked it?

@jocel1
Copy link
Contributor Author

jocel1 commented Aug 1, 2017

Only if there's no preexisting shop-specific json files. (it occurs after an upgrade for example)
The files are only created when ThemeManager->saveTheme() is called

@Quetzacoalt91
Copy link
Member

I'm wondering if the other unlink/dump occurrences of these Json files are useful, because they will be created by this getInstanceByName() anyway.

By the way, could we add a check with the file timestamps? This would allow us to regenerate the json file as soon as the YML file is modified. And we can also check the environment in order to to this only in dev mode.
I'm open to any discussion, because this suggestion may break something. I do not own this part of the project. 😬

@jocel1
Copy link
Contributor Author

jocel1 commented Aug 1, 2017

right about the cache invalidation. I can modify as well saveTheme() to just invalidate the cache instead of regenerating it.

@jocel1
Copy link
Contributor Author

jocel1 commented Aug 2, 2017

After reading again the code, I don't think my fix introduce any potential regression.
The old "normal" behaviour is to generate the json cache file once the theme is saved (through the ThemeManager->saveTheme() call). Once the cache file is generated, it's never invalidated.

However, I need to check why the saveTheme() is not called after a migration, the real fix which needs to be done is perhaps here.

@aleeks aleeks changed the base branch from 1.7.2.x to develop August 2, 2017 07:45
@aleeks
Copy link
Contributor

aleeks commented Aug 2, 2017

Hello @jocel1
It's not really a bug fix so i move this PR on developbranch, you need to rebase from develop!
Thank you!

@Quetzacoalt91
Copy link
Member

Once the cache file is generated, it's never invalidated.

Yeah, and it seems it could be better here. Once you import a theme, the data modified in the yml config file is never taken in account, which keeps stale data loaded during the development.

@jocel1
Copy link
Contributor Author

jocel1 commented Aug 3, 2017

Actually I have checked, and the issue only occurs if the upgrade is run in command line without a theme upgrade. The case in this PR will probably never happen for real. But yet it could be useful to create a JIRA to track the cache invalidation issue

@Quetzacoalt91 Quetzacoalt91 merged commit 74f2e15 into PrestaShop:develop Aug 10, 2017
@Quetzacoalt91
Copy link
Member

Thank you @jocel1

@xBorderie xBorderie modified the milestones: 1.7.2.3, 1.7.3.0 Sep 1, 2017
@jocel1
Copy link
Contributor Author

jocel1 commented Oct 8, 2017

Actually the case in this PR occurs for real, when the cache is purged, or the codebase is redeployed without any cache files. In this case the theme cache is never regenerated, and the application performance is poor (I've encounter the case with a real customer).
@eternoendless It could perhaps make sense to merge it in 1.7.2.x since it's a bug.

@eternoendless
Copy link
Member

There will be no more releases for 1.7.2.x except for critical security fixes.

@kpodemski
Copy link
Contributor

That's a shame, if we clear the cache, we loose columns settings, also undefined variables during AJAX requests, these are critical bugs...

@eternoendless
Copy link
Member

@kpodemski we can discuss about it on Gitter if you want to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants