In TwigEnvironment::updateCompiledTemplate, an attempt is made to save the compiled template. However, it does not check the return value of `$this->storage()->save`, which can return false.

This results in the assumption that the new file got written out, so it is loaded with `$this->storage()->load` (return value not checked here either), and then a fatal error when `new` is called for the new class.

I ran into this by changing the twig cache directory to a directory which was not writable. There was no indication that that was my problem, just a fatal error.

Files: 
CommentFileSizeAuthor
#6 2481427-6.patch830 bytesCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,018 pass(es). View

Comments

dawehner’s picture

Its good that people actually try those things out!

dawehner’s picture

Its good that people actually try those things out!

Fabianx’s picture

dawehner’s picture

Issue tags: +Needs tests

The other issue should not help here, as its about not a fix in TwigEnvironment itself.
No matter what our php storage implementation does, its a bug that we ignore the return value of save() and carry on as if nothing happened.

I guess we should trigger an error in case save() return FALSE.

Cottser’s picture

Assigned: Unassigned » Cottser
Issue tags: +Twig

Going to look into this en route to LA.

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Active » Needs review
FileSize
830 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,018 pass(es). View

Attached a very rough proof of concept of throwing errors when saving and it seems to work.

mgifford’s picture

Status: Needs review » Needs work

Needs re-roll.

Cottser’s picture

I suspect this may have been fixed by #2568171: Upgrade to Twig 1.22 and implement our own cache class but even if so it could definitely use another manual test to ensure that's the case, and potentially additional test coverage.

harshil.maradiya’s picture

issue has been solved in this ticket
#2568171: Upgrade to Twig 1.22 and implement our own cache class

harshil.maradiya’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

@harshil.maradiya the correct status here is to close this as a duplicate of #2568171: Upgrade to Twig 1.22 and implement our own cache class