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.

Sign up for GitHub

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

Jump to bottom

Fix race condition in Environment::loadTemplate (1.x) #1840

Merged
merged 1 commit into from
Sep 21, 2015
Merged

Fix race condition in Environment::loadTemplate (1.x) #1840

merged 1 commit into from
Sep 21, 2015

Conversation

znerol
Copy link
Contributor

@znerol znerol commented Sep 20, 2015

Fixes #1836 (1.x branch)

@znerol znerol mentioned this pull request Sep 20, 2015
Closed
@znerol znerol changed the title Fix race condition in Environment::loadTemplate Fix race condition in Environment::loadTemplate (1.x) Sep 20, 2015
@LionsAd
Copy link

LionsAd commented Sep 20, 2015

This looks pretty great to me.

Probably has() would need to be retained as deprecated function though - due to BC.

@fabpot
Copy link
Contributor

fabpot commented Sep 20, 2015

has() has just been added, so no need to keep BC here. I will review this PR tomorrow or later this week, during DrupalCon :) See you there.

@stof
Copy link
Member

stof commented Sep 20, 2015

@fabpot BC is needed. The Twig_CacheInterface has already been released.

@fabpot
Copy link
Contributor

fabpot commented Sep 20, 2015

No need to keep BC when the interface has been released a week ago. Let's not add overhead for no good reasons.

@fabpot fabpot merged commit 1bb7000 into twigphp:1.x Sep 21, 2015
fabpot added a commit that referenced this pull request Sep 21, 2015
@fabpot
bug #1840 Fix race condition in Environment::loadTemplate (1.x) (zn…
93914e5 
…erol)

This PR was merged into the 1.x branch.

Discussion
----------

Fix race condition in `Environment::loadTemplate` (1.x)

Fixes #1836 (1.x branch)

Commits
-------

1bb7000 Fix race condition in `Environment::loadTemplate`
@fabpot
Copy link
Contributor

fabpot commented Sep 21, 2015

Thanks @znerol for working on this issue.

@fabpot fabpot mentioned this pull request Sep 21, 2015
Closed
@Tobion Tobion mentioned this pull request Sep 24, 2015
Closed
fsw added a commit to fsw/Twig that referenced this pull request Sep 24, 2015
@fsw
use is_file instead of error suppression operator
ce226d8 
it worked this way before before twigphp#1840
avoid error suppress operator when there is no good reason for it.
@fsw fsw mentioned this pull request Sep 24, 2015
Closed
drzraf referenced this pull request Dec 9, 2016
@fabpot
bug #2043 Fix load cache (MaCroTux)
c561e89 
This PR was squashed before being merged into the 1.x branch (closes #2043).

Discussion
----------

Fix load cache

In PHP7 @ do not works and it is resource expensive, switching to if statement.

Commits
-------

b5df3d1 Fix load cache
@drzraf
Copy link

drzraf commented Dec 9, 2016

After looking at
#2043
I wonder:

  • isn't this commit reintroducing the race-condition that the file_exists() was expected to fix?
  • why is @ needed? If PHP die on buggy generated code? it would be best to let the error flow out as much as PHP permits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@znerol @LionsAd @fabpot @stof @drzraf @TheRatG