Skip to content

[TwigBundle] Reconfigure twig paths when they are updated #14778

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

Closed
wants to merge 1 commit into from
Closed

[TwigBundle] Reconfigure twig paths when they are updated #14778

wants to merge 1 commit into from

Conversation

chbruyand
Copy link
Contributor

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #14771, #14768, #14262
License MIT

Directory's content and modification time are not considered any more for freshness check. Only it's creation or deletion are now checked.
This addresses performance issues (#14771, #14768), and also improves the cases were container is reconfigured as directory creation needed clearing the cache manually (#14262).

Directory's content and modification time are not considered any more
for freshness check. Only it's creation or deletion are now checked.
This addresses performance issues (#14771, #14768), and also improves
the cases were container is reconfigured as directory creation needed
clearing the cache manually (#14262).
@mpdude
Copy link
Contributor

mpdude commented May 29, 2015

SemVer wise, it would be better to have this in 2.7 or 2.8 so others can depend on that minor version bump if they want to use the new resource type.

I assume you based it on 2.3 because that already merged (and released?) a change that causes the performance problem?

@chbruyand
Copy link
Contributor Author

Yes, #14262 has been merged, and released in 2.3.29 and 2.6.8. see #14771, #14768

@mpdude
Copy link
Contributor

mpdude commented May 29, 2015

So not quite sure, but maybe src/Symfony/Bundle/TwigBundle/composer.json needs an update then to require symfony/config >= 2.3.30? What about Symfony 2.5?

@stof
Copy link
Member

stof commented May 29, 2015

Adding new features in the Config component in a patch release is a no-go. It will be very hard to write a constraint to use this new feature (it would be ~2.3.30|~2.6.9|~2.7, which is not something we would like to advocate).
We should respect semver and add the new feature in 2.8 only IMO.

@mpdude
Copy link
Contributor

mpdude commented May 29, 2015

If the performance impact is huge, that might include reverting the changes mentioned above, which have already been released in a bugfix.

@chbruyand
Copy link
Contributor Author

OK, so you revert #14262 and I add this patch, as a feature, on branch 2.8 ?

@JeremyIe3
Copy link

It might not be of any help, but i'm also experiencing bad performance in dev env since version 2.3.29, and the mentionned commit clearly is responsible of this behavior.

@fabpot
Copy link
Member

fabpot commented May 29, 2015

We should:

@fabpot
Copy link
Member

fabpot commented May 29, 2015

PR reverted

@bdronneau
Copy link

A new 2.3.x will be tag ?

@chbruyand
Copy link
Contributor Author

Did make the proper PR on 2.8 in #14781.

I'm closing this PR.

@chbruyand chbruyand closed this May 29, 2015
@chbruyand chbruyand deleted the TICKET_14262 branch May 29, 2015 15:09
fabpot added a commit that referenced this pull request Jun 5, 2015
…ted (chbruyand)

This PR was squashed before being merged into the 2.8 branch (closes #14781).

Discussion
----------

[TwigBundle] Reconfigure twig paths when they are updated

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #14771, #14768, #14262, #14778
| License       | MIT

Refresh twig paths upon creation and deletion. As we don't care neither about path's modification time nor path's content, a new Resource has been added in the Config Component.
Full discussion in #14778.

Commits
-------

3cbff05 [TwigBundle] Reconfigure twig paths when they are updated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants