{"action":"created","issue":{"url":"https://api.github.com/repos/twigphp/Twig/issues/2697","repository_url":"https://api.github.com/repos/twigphp/Twig","labels_url":"https://api.github.com/repos/twigphp/Twig/issues/2697/labels{/name}","comments_url":"https://api.github.com/repos/twigphp/Twig/issues/2697/comments","events_url":"https://api.github.com/repos/twigphp/Twig/issues/2697/events","html_url":"https://github.com/twigphp/Twig/issues/2697","id":329084609,"node_id":"MDU6SXNzdWUzMjkwODQ2MDk=","number":2697,"title":"How is auto reload supposed to work?","user":{"login":"ericmorand","id":125664,"node_id":"MDQ6VXNlcjEyNTY2NA==","avatar_url":"https://avatars0.githubusercontent.com/u/125664?v=4","gravatar_id":"","url":"https://api.github.com/users/ericmorand","html_url":"https://github.com/ericmorand","followers_url":"https://api.github.com/users/ericmorand/followers","following_url":"https://api.github.com/users/ericmorand/following{/other_user}","gists_url":"https://api.github.com/users/ericmorand/gists{/gist_id}","starred_url":"https://api.github.com/users/ericmorand/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ericmorand/subscriptions","organizations_url":"https://api.github.com/users/ericmorand/orgs","repos_url":"https://api.github.com/users/ericmorand/repos","events_url":"https://api.github.com/users/ericmorand/events{/privacy}","received_events_url":"https://api.github.com/users/ericmorand/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2018-06-04T14:39:48Z","updated_at":"2018-06-05T09:10:22Z","closed_at":"2018-06-04T17:35:42Z","author_association":"NONE","body":"Hi guys.\r\n\r\nWe faced a bug recently with Twing where the auto_reload option was not working as expected:\r\n\r\nhttps://github.com/ericmorand/twing/issues/194\r\n\r\nIt was fixed by changing the loadTemplate method of our TwingEnvironment class, which is analoguous to TwigPHP Twig_Environment one.\r\n\r\nMade me wonder how auto_reload can be working in TwigPHP:\r\n\r\n```\r\npublic function loadTemplate($name, $index = null)\r\n    {\r\n        $cls = $mainCls = $this->getTemplateClass($name);\r\n        if (null !== $index) {\r\n            $cls .= '_'.$index;\r\n        }\r\n\r\n        if (isset($this->loadedTemplates[$cls])) {\r\n            return $this->loadedTemplates[$cls];\r\n        }\r\n        \r\n        if (!class_exists($cls, false)) {\r\n            $key = $this->cache->generateKey($name, $mainCls);\r\n\r\n            if (!$this->isAutoReload() || $this->isTemplateFresh($name, $this->cache->getTimestamp($key))) {\r\n                $this->cache->load($key);\r\n            }\r\n            // ...\r\n    }\r\n```\r\n\r\nWhen I read the code, I can't unsee that if the template has already been loaded previously, it is returned as is and not be given the chance to be reloaded.\r\n\r\nIs there something I'm missing there? In Twing, we had to put the `loadedTemplates` check inside the `isAutoReload `check. to fix the bug. But I'm not sure it is the correct way to fix that, maybe the bug is coming from somewhere else.\r\n\r\nWhat do you think? "},"comment":{"url":"https://api.github.com/repos/twigphp/Twig/issues/comments/394638514","html_url":"https://github.com/twigphp/Twig/issues/2697#issuecomment-394638514","issue_url":"https://api.github.com/repos/twigphp/Twig/issues/2697","id":394638514,"node_id":"MDEyOklzc3VlQ29tbWVudDM5NDYzODUxNA==","user":{"login":"ericmorand","id":125664,"node_id":"MDQ6VXNlcjEyNTY2NA==","avatar_url":"https://avatars0.githubusercontent.com/u/125664?v=4","gravatar_id":"","url":"https://api.github.com/users/ericmorand","html_url":"https://github.com/ericmorand","followers_url":"https://api.github.com/users/ericmorand/followers","following_url":"https://api.github.com/users/ericmorand/following{/other_user}","gists_url":"https://api.github.com/users/ericmorand/gists{/gist_id}","starred_url":"https://api.github.com/users/ericmorand/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ericmorand/subscriptions","organizations_url":"https://api.github.com/users/ericmorand/orgs","repos_url":"https://api.github.com/users/ericmorand/repos","events_url":"https://api.github.com/users/ericmorand/events{/privacy}","received_events_url":"https://api.github.com/users/ericmorand/received_events","type":"User","site_admin":false},"created_at":"2018-06-05T09:10:22Z","updated_at":"2018-06-05T09:10:22Z","author_association":"NONE","body":"@stof, you're right. I had to change the way auto_reload works to fix the bug.\r\n\r\nBut I then discovered that the \"one fresh instance per request - or per build round-trip if you think about a Node process that would watch files\" is the best way to ensure a consistent and reliable behavior.\r\n\r\nFor example, there is no easy way to catch changes on extended templates without restarting with a fresh instance. When a template has a parent, this parent is loaded in the *constructor* of the template. It means that, with auto_reload set to true, if the parent template change, it is never given the chance to be reloaded since loadTemplate is never called on him. It may be called on the child template itself but since the child template didn't change (only its parent changed), it's resolved from the local environment cache (loadedTemplates) which contains already instanciated templates. Thus the template constructor is not called.\r\n\r\nI don't want to mess with TwigPHP logic. First because it works perfectly, is realiable and has been tested and improved over the years. Second because I want to stick with my principle of being the closest possible to your implementation.\r\n\r\nThat's why I reverted the fix. And everything is working wonderfully as long as we create a fresh Twing instance before each subsequent build.\r\n\r\n\r\n\r\n"}}
{"actor":{"display_login":"ericmorand"}}