Skip to content

changed the arguments of generateKey#1822

Merged
fabpot merged 2 commits intotwigphp:1.xfrom
fabpot:template-cache-tweaks
Sep 12, 2015
Merged

changed the arguments of generateKey#1822
fabpot merged 2 commits intotwigphp:1.xfrom
fabpot:template-cache-tweaks

Conversation

@fabpot
Copy link
Copy Markdown
Contributor

@fabpot fabpot commented Sep 12, 2015

We now pass the template name and the template class, which seems much better from a UX standpoint.

Passing the prefix was really just a hack and an implementation leak to be able to determine the variable part of the class name and generate sub-directories for templates. Now, we generate the cache key by taking the last characters instead of the first ones, to avoid the need for the class prefix.

That should also make Drupal happy :)

@fabpot
Copy link
Copy Markdown
Contributor Author

fabpot commented Sep 12, 2015

I've also deprecated Twig_Environment::getTemplateClassPrefix() as this is really just an implementation detail people should not rely on.

@fabpot fabpot force-pushed the template-cache-tweaks branch from 97a5f00 to 5f491b5 Compare September 12, 2015 09:38
Comment thread lib/Twig/Cache/Filesystem.php Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The class name might contain the index with a _x suffix. In this case the directory structure is not randomized evenly anymore. A simple solution would be to just change ´getTemplateClass` to add the index between prefix and hash.
But this still ties to Cache implementation to the class generation, i.e. the cache is basically aware how the class name is generated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An architectural better solution would be to hash the class name again and then use two chars from the hash. This would also solve the problem when strlen($className) < 2.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea, code updated accordingly.

@fabpot fabpot force-pushed the template-cache-tweaks branch from 5f491b5 to c2e75ff Compare September 12, 2015 13:35
@star-szr
Copy link
Copy Markdown
Contributor

Thanks very much @fabpot, will take this for a spin shortly.

@star-szr
Copy link
Copy Markdown
Contributor

Code looks good to me and this unblocked my work, thanks again. Remaining steps for me are:

  1. Test race condition or reach out to others who can (https://www.drupal.org/node/2429659), I'm hoping it no longer needs special handling.
  2. Test with Twig 2.x.

@fabpot fabpot merged commit c2e75ff into twigphp:1.x Sep 12, 2015
fabpot added a commit that referenced this pull request Sep 12, 2015
This PR was merged into the 1.x branch.

Discussion
----------

changed the arguments of generateKey

We now pass the template name and the template class, which seems much better from a UX standpoint.

Passing the prefix was really just a hack and an implementation leak to be able to determine the variable part of the class name and generate sub-directories for templates. Now, we generate the cache key by taking the last characters instead of the first ones, to avoid the need for the class prefix.

That should also make Drupal happy :)

Commits
-------

c2e75ff changed the arguments of generateKey
0157315 deprecated Twig_Environment::getTemplateClassPrefix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants