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

Create Timber-Scanner #2

Open
oscarotero opened this issue May 1, 2020 · 2 comments
Open

Create Timber-Scanner #2

oscarotero opened this issue May 1, 2020 · 2 comments

Comments

@oscarotero
Copy link
Member

The idea is that this repository should contain only the general code to scan Twig templates and create a different package gettext/timber-scanner with the other stuff needed for timber. Advantages:

  • Timber use Twig v2 instead v3. Twig scanner could have different versions in order to support both versions. For example, twig-scanner v1 (twig 2) and twig-scanner v2 (twig 3).
  • Remove timber as a dependency of twig-scanner, so users that want to use Twig-scanner are not forced to install it.
  • We can create other Twig-related scanners, for example to support i18n extension.

@drzraf What do you think?

If you're ok, can you please create a new package? Or if you cannot, let me know and I can do it.

@drzraf
Copy link
Collaborator

drzraf commented May 1, 2020

tldr; I'd suggest to wait to see what's the next Twig-implementation looks like first, in order to avoid the complexity of an over-anticipated factorization.

  • Timber dependency forcefully pulls Twig 2, but for dev only so nothing blocking users from using it with any Twig version. Still, this could be improved by putting Timber-specific in the wp-cli i18-command package. Twig-Scanner can load arbitrary extension (or arbitrary Twig Environment) previous to parsing, then the tests could just use a fixture for its own Twig extension/function/filter. Just keep in mind that Timber is a Twig extension (as is the i18n extension) except that it still lacks the formal addExtension mechanism.
  • TwigFunctionsScanner is not that generic (yet). It only extracts ParsedFunction based on Twig FunctionExpression. But if we were to support the {% trans %} tag, the current parser would not fit. A CMS like Grav preferred a t Twig filter.
  • For a simple WP theme translation, there are already 5 packages involved:
    • wp-cli
    • wp-cli/i18n-command (bridges with php-gettext)
    • php-gettext
    • php-gettext/twig-scanner
    • timber
      By moving carefully code chunks to the right places we could avoid adding 1 or 2 other packages to that stack.

I suggest to first wait a couple of days for the full stack to be done, test (I'd like to regenerate my own POT files with the new stack), and play with wp-cli/Timber initialization and maybe fix wp-cli i18n-command gettext-5 support down the road and then having a look back :)

@oscarotero
Copy link
Member Author

Yes, sure. There's no rush 😄

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

No branches or pull requests

2 participants