Implemented #EZP-20905 : Check filemtime for templates

[tharkun]

Here comes a proposal of enhancement so we can stop checking mtime of template files.

https://jira.ez.no/browse/EZP-20905

[gggeek]

I would add a +10 iff you add as well an option to eliminate completely the check for file_exists(). As long as we can build the name of the cached-version of the file and check if it is there without having to check if original tpl is there, this allows us to deploy on tpl caches without even having original tpls on delivery servers!

[tharkun]

If you want me to, i could add a second constant called EZP_TEMPLATE_FILEEXISTS_CHECK to avoid doing a file_exists on the template file.

Should i add this feature ?

[gggeek]

I'd actually use ini settings for both - since we normally bootstrap ini system before tpl one

[tharkun]

You're right. I'll create two settings in site.ini[TemplateSettings].

[tharkun]

Well, here is two new settings to avoid checking whether template files exists or not !

[bdunogier]

Note about commit messages: we haven't really written this down yet, but we will most likely impose less restrictions . You can make it shorter, like "EZP-20905: Updated this and that". This allows us to have more meaningful commits, since we maintain the changelogs using JIRA now.

[andrerom]

Besides the nitpicpick questions, there are some years since I introduced the EZP_INI_FILEMTIME_CHECK flag, but one question: On a well cached page (cache blocks and view cache) you will most likely only save one stat call for pagelayout unless DevelopmentMode is enabled? If so, is this worth it? It complicates things, especially regarding what seems to now be a gotcha we end up with when you enable DevelopmentMode but disables TemplateCheckMTime.

[gggeek]

@andrerom all ez caches do expire now & then, so avg nr. of stat calls saved is higher (think also view cache w. ttl 0)

[andrerom]

cache_ttl = 0 is not really best practice, and if you have enabled that your performance is already sub par.

[tharkun]

Above all the websites we manage at work, none use DevelopmentMode on production (even on our PCs).

You're right, it would save one stat call on each called page, but let's think on a mutual plateform with big website, it would save a huge amount of stat call.

To avoid problems with DevelopmentMode, we could add set TemplateCheckMTime && TemplateCheckFileExists to true if DevelopmentMode is enabled, so it would not be an issue any more.

[andrerom]

To avoid problems with DevelopmentMode, we could add set TemplateCheckMTime && TemplateCheckFileExists to true if DevelopmentMode is enabled, so it would not be an issue any more.

Yes that is the kind of logic that needs to be there I think, otherwise it can lead to some issues.

[tharkun]

Is every thing good about this PR ?

[tharkun]

@andrerom : is every thing good for you now ?

[lolautruche]

Wow, thanks @guillaumelecerf for doing the review :smile:

[guillaumelecerf]

@tharkun Good job ! Everything seems now syntactically correct, but I did not run-tested it.

[guillaumelecerf]

:+1:

[guillaumelecerf]

I think it needs now to be rebased in a single commit before merging, @andrerom @bdunogier @lolautruche @gggeek ?