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.

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

[FrameworkBundle] Use relative paths in templates paths cache #19687

Merged
merged 1 commit into from
Aug 23, 2016

Conversation

tgalopin
Copy link
Contributor

@tgalopin tgalopin commented Aug 20, 2016

Q A
Branch? master
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #3079
License MIT
Doc PR -

This implements the usage of relative paths instead of absolute ones in var/cache/*/templates.php, important for ability to build the cache in a different context than where it will be used.

This PR transforms the following templates.php:

<?php return array (
  ':default:index.html.twig' => '/home/tgalopin/www/symfony-standard/app/Resources/views/default/index.html.twig',
  '::base.html.twig' => '/home/tgalopin/www/symfony-standard/app/Resources/views/base.html.twig',
);

Into:

<?php return array (
  ':default:index.html.twig' => __DIR__.'/../../../app/Resources/views/default/index.html.twig',
  '::base.html.twig' => __DIR__.'/../../../app/Resources/views/base.html.twig',
);

I also added tests for the TemplateCachePathsWarmer and improved tests for the TemplateLocator.

@@ -23,20 +23,25 @@ class TemplateLocator implements FileLocatorInterface
{
protected $locator;
protected $cache;
protected $rootDir;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be private I think.

@tgalopin tgalopin force-pushed the templates-cache-relative-paths branch 5 times, most recently from 6aec93e to 2bb53d3 Compare August 21, 2016 09:40
@tgalopin
Copy link
Contributor Author

Travis failure is not related (PHP 7.1 segfault).

$path = $this->locator->locate($template->getPath(), $currentPath);

if ($this->rootDir && 0 === strpos($path, $this->rootDir)) {
return $this->rootDir.($this->cache[$key] = './'.substr($path, strlen($this->rootDir)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: As the length of the rootDir doesn't change at runtime you can cache it as a property or static local variable.

@tgalopin tgalopin force-pushed the templates-cache-relative-paths branch 5 times, most recently from 5276db5 to 4f601e8 Compare August 22, 2016 08:59
@sstok
Copy link
Contributor

sstok commented Aug 22, 2016

LGTM.

@tgalopin tgalopin force-pushed the templates-cache-relative-paths branch from 4f601e8 to 6f6139c Compare August 22, 2016 12:31
@nicolas-grekas
Copy link
Member

👍

@@ -32,7 +32,7 @@ class TemplateLocator implements FileLocatorInterface
*/
public function __construct(FileLocatorInterface $locator, $cacheDir = null)
{
if (null !== $cacheDir && is_file($cache = $cacheDir.'/templates.php')) {
if (null !== $cacheDir && file_exists($cache = $cacheDir.'/templates.php')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file_exists is faster and is_file have no advantage over it here.

@dunglas
Copy link
Member

dunglas commented Aug 23, 2016

👍

@fabpot
Copy link
Member

fabpot commented Aug 23, 2016

Thank you @tgalopin.

@fabpot fabpot merged commit 6f6139c into symfony:master Aug 23, 2016
fabpot added a commit that referenced this pull request Aug 23, 2016
…s cache (tgalopin)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[FrameworkBundle] Use relative paths in templates paths cache

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #3079
| License       | MIT
| Doc PR        | -

This implements the usage of relative paths instead of absolute ones in `var/cache/*/templates.php`, important for ability to build the cache in a different context than where it will be used.

This PR transforms the following `templates.php`:

``` php
<?php return array (
  ':default:index.html.twig' => '/home/tgalopin/www/symfony-standard/app/Resources/views/default/index.html.twig',
  '::base.html.twig' => '/home/tgalopin/www/symfony-standard/app/Resources/views/base.html.twig',
);
```

Into:

``` php
<?php return array (
  ':default:index.html.twig' => __DIR__.'/../../../app/Resources/views/default/index.html.twig',
  '::base.html.twig' => __DIR__.'/../../../app/Resources/views/base.html.twig',
);
```

I also added tests for the TemplateCachePathsWarmer and improved tests for the TemplateLocator.

Commits
-------

6f6139c [FrameworkBundle] Use relative paths in templates paths cache
@tgalopin tgalopin deleted the templates-cache-relative-paths branch August 23, 2016 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants