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

Fix timestamps for caching incorrect #3006

Merged

Conversation

@carakas
Copy link
Member

carakas commented Dec 17, 2019

Type

  • Enhancement
  • Deprecation

Pull request description

Use the filetime of the actual file instead of that of the parameters.yml file for caching assets
That way you don't need to touch the parameters.yml file to make sure your assets get a new url

@carakas carakas added this to the 5.7.2 milestone Dec 17, 2019
@carakas carakas requested a review from forkcms/core-contributors Dec 17, 2019
@carakas carakas added this to In progress in Restructure Core via automation Dec 17, 2019
@carakas carakas added the deprecation label Dec 17, 2019
return $this->file . (mb_strpos($this->file, '?') === false ? '?' : '&') . 'm=' . LAST_MODIFIED_TIME;
$separator = mb_strpos($this->file, '?') === false ? '?' : '&';

return $this->file . $separator . 'm=' . filemtime(__DIR__ . '/../../../../' . $this->file);

This comment has been minimized.

Copy link
@tijsverkoyen

tijsverkoyen Dec 18, 2019

Member

I don't know if this is a correct solution:

  1. The results of this function are cached. See clearstatcache() for more details
  2. In order to have a reliable modified time you need to execute clearstatcache before, only once per request.

The reason why we have chosen the modified time from parameters.yml (or globals.php in much older versions) was because we could touch a single file in deploy-scripts to change the version on all assets.

This comment has been minimized.

Copy link
@carakas

carakas Dec 18, 2019

Author Member

@tijsverkoyen I looked at that function because I initially thought the same.
I actually tested it locally and it works.
When you look deeper in the clearstatcache function it says that it is withing the same request.
It is there in case a file can be changed during the request itself or long running cronjobs. And in case of the assets that won't happen

This comment has been minimized.

Copy link
@tijsverkoyen

tijsverkoyen Dec 20, 2019

Member

I don't get it. Again: In order to have a reliable modified time you need to execute clearstatcache before, only once per request.

So will you call clearstatcache on each request, to have reliable timestamps? If so: this will kill performance as the stat cache will be cleared a lot and therefore be useless. Of not: the timestamps won't be reliable. It probably works locally as it is correctly implemented on MacOS.
As I said above we chose in the past to touch the parameters file for that reason.
But maybe others should give their opinion.

This comment has been minimized.

Copy link
@carakas

carakas Dec 27, 2019

Author Member

Ah oke, I see now, I thought that cache was only kept during the current request

This comment has been minimized.

Copy link
@carakas

carakas Dec 27, 2019

Author Member

What would you suggest instead? hashing the file? But that would probably be rather slow

This comment has been minimized.

Copy link
@tijsverkoyen

tijsverkoyen Dec 27, 2019

Member

They probably call clearstatcache on upload to get an non-cached version of the size. Uploading files does not happen on each request, so this won't be a performance killer.

I wasn't aware Spoon/Symfony did a clearstatcache on upload, so in essence this means that your method will work for assets uploaded thru Spoon/Symfony (in essence: all modules + media library).

For assets added thru SSH/FTP/deployment it can happen that the cache is not cleared if there is no file uploaded. But that could easily be fixed by adding a clearstatcache on deployment.

This comment has been minimized.

Copy link
@carakas

carakas Dec 27, 2019

Author Member

Good point about the media library.
We would indeed then need to add it to the deployment, or we can add it to the forkcms:clear:cache command?

This comment has been minimized.

Copy link
@tijsverkoyen

tijsverkoyen Dec 27, 2019

Member

It would make sense to add in into forkcms:clear:cache.

Also, another setback:

Note that, unlike APCu and Opcache, the file status cache is per-process rather than stored in shared memory. This means that running stat:clear against PHP-FPM will only affect whichever FPM worker responds to the request, not the whole pool.
Source: https://gordalina.github.io/cachetool/

stat:clear essential runs clearstatcache. So on servers running PHP FPM (which most hosters are using) it probably still isn't reliable ;-)

I would think there is a reason why Symfony does not have a default cache strategy based on modified time

I think the most realistic approach is:

  1. Using the editedOn-field as a version string for assets that are uploaded with the MediaLibrary.
  2. Convert core modules to use the MediaLibrary instead of own upload mechanism.
  3. Implement Encore and enable versioning

This comment has been minimized.

Copy link
@StijnVrolijk

StijnVrolijk Jan 3, 2020

Contributor

+1 for adding it to forkcms:cache:clear. It makes the most sense.

This comment has been minimized.

Copy link
@carakas

carakas Jan 16, 2020

Author Member

fixed in e5feef6
@tijsverkoyen but that issue already exists as we use the exact same system on the parameters.yml afaik

Restructure Core automation moved this from In progress to Reviewer approved Jan 17, 2020
@carakas carakas merged commit 0c3b4f7 into forkcms:master Jan 30, 2020
4 checks passed
4 checks passed
Scrutinizer Analysis: No new issues – Tests: passed
Details
TypoCheck No typos found
Details
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Restructure Core automation moved this from Reviewer approved to Done Jan 30, 2020
@carakas carakas deleted the justcarakas:fix-timestamps-for-caching-incorrect branch Jan 30, 2020
@carakas carakas modified the milestones: 5.7.2, 5.8.0 Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.