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

Functions scanner for Twig AST #59

Closed
wants to merge 1 commit into from

Conversation

drzraf
Copy link

@drzraf drzraf commented Jul 27, 2018

I would have preferred to submit this to upstream oscarotero/Gettext but the way i18n function wrappers work make it a bit difficult (It's easier to inherit from wp-cli PhpFunctionsScanner)

Note: This extractor work at a "higher" level than current one (which usually run a PHP extractor after a Twig-compilation pass).
#52

@schlessera
Copy link
Member

I'm open to removing the final keyword from PhpFunctionsScanner for now.

However, as already stated in #52 (comment), I don't think Twig support is a good fit for bundling right now.

Also, please note that we don't accept code that comes with individual copyright or changes licensing.

@drzraf
Copy link
Author

drzraf commented Jul 31, 2018

  • final keyword: Make PhpFunctionsScanner extensible #62
  • about license, I wrongly believed that wp-cli was GPL licenced excepted this package (which relies upon an MIT-licensed lib'). Dual-license header removed (I hope in the future wp-cli could be GPL-ed, if not dual-licenced btw :))
  • About Twig support. I tried to formalize the concerns in ease inheritance in order to support Twig/Timber #52 (comment) (the main one was that Twig support would complexify command-line, especially argument handling/bootstrap level). I'm still trying to solve all the issues resulting from keeping this Twig support outside i18n package. But if it's not possible, I still believe that including the class wouldn't harm the package either: it'd just make one public class available to external commands (and it'd be logical since scanner/extractor rely more on i18n package than on anything else and was written so that it share the same logic).

@schlessera
Copy link
Member

  • re. final: merged.
  • re. license: MIT is compatible with GPL. Not sure what benefit the more restrictive GPL would add to this project, apart from blocking some hosting companies from actually using it.
  • re. Twig support: If there are currently limitations that don't allow an easy extension to support Twig, then we should work on removing these limitations. WP-CLI is meant to be extensible from the ground up, and I heartily welcome any improvements we can make in that regard.

@schlessera
Copy link
Member

As #62 has been merged, I'm closing this pull request now. Getting rid of extensibility limitations to make third-party Twig support viable can be done in a fresh PR.

@schlessera schlessera closed this Jul 31, 2018
@drzraf
Copy link
Author

drzraf commented Jul 31, 2018

I updated #52 (comment).
You will find there the limitations that keep me from inheriting make-pot MakePotCommand from a theme-defined subcommand.

@drzraf
Copy link
Author

drzraf commented Jul 31, 2018

@oscarotero: would it fit better there?

@oscarotero
Copy link

@drzraf I'm not a twig user so don't get the difference between this extractor and the one currently in Gettext.

@drzraf
Copy link
Author

drzraf commented Jul 31, 2018

Gettext current one:

  1. Transforms Twig as PHP code (= tokenize()+parse()+render()
  2. Uses a PhpCode extractor (and its specific configuration about function name)

advantages:

  • Ability to reuse PhpCode extractor to extract function call

disadvantage:

  • Twig rendered php code can be transformed/wrapped thus hiding gettext functions. See Don't overwrap WordPress i18n functions timber/timber#1753. In that case, PhpCode won't find the calls to __(), gettext(), ... because they are wrapped inside call_user_func(), ...
  • Can't currently handle templates making use of custom Twig extensions.

Proposed one:

  1. Parse Twig generated AST tree (= tokenize()+parse())
  2. Recursively iterate over the tree to find function calls.

advantages:

  • Narrower to Twig syntax. Eg: {{ __("foo", "domain") }}. AST has undergone less transformations so function wrapping isn't problem anymore
  • More robust because it iterates over the AST from Twig parser, instead of looking at source-code
  • Probably more efficient
  • Since it takes an already initialized $twig environment in the constructor instead of a string, it supports user-defined extensions ¹

disadvantages:

  • Can't rely upon PhpCode extractor anymore
  • Configuration of gettext() function names to extract (__, _x, _e, ngettext, ...) needs to be known sooner than currently = at the functionsScanner level step.

¹ It's simple (and thus common) to extend Twig with additional filters/functions. (That's what Timber does for handling Twig i18n the-wordpress-way). But all these filters and functions must be defined before parsing the template. A good scanner+extractor must let user hook in the process to register their extensions on the Twig parser.

@oscarotero
Copy link

@drzraf Ok, it looks fine to me.

I don't care about don't reusing PhpCode. It was created originally to scan php code, not to reuse by other template based extractors. I only have this suggestions:

  • In this PR, you extended PhpFunctionScanner but without use any of the inherited methods, so it's useless. Due this code is specific for Twig and won't be used by other extractors, I'd put it directly in the Twig extractor
  • In the extractor you can pass the initialized $twig and the functions names in the $options argument.

If you want to work in a pull request, I'd appreciate that.
Thanks!

drzraf pushed a commit to drzraf/Gettext that referenced this pull request Oct 28, 2018
Current Twig parser operate by
1. Transforming Twig as PHP code = tokenize() + parse() + render()
2. Using a PhpCode extractor (and its specific configuration about function name)

Disadvantage:
* Twig rendered PHP code can be transformed/wrapped in call_user_func() making that gettext functions undetectable by PhpCode extractor.
  (See for example timber/timber#1753).
* Can't handle templates making use of custom Twig extensions (very common)

This patch offer an extractor that:
* Parse Twig generated AST tree (= tokenize()+parse())
* Recursively iterate over node three to find function gettext calls.

Advantages:
* Operating sooner, at the AST level, Twig expressions like `{{ __("foo", "domain") }}` aren't yet wrapped.
* More robust because it directly iterates over the AST from Twig parser instead of looking at PHP source-code.
* Supports initialized `$twig` environment, thus supporting user-defined extensions?
* Possibly more efficient.

Ref: wp-cli/i18n-command#59
@drzraf
Copy link
Author

drzraf commented Oct 28, 2018

FTR: PR at php-gettext/Gettext#195

@drzraf drzraf mentioned this pull request May 4, 2020
@drzraf drzraf mentioned this pull request Jun 18, 2020
@drzraf drzraf mentioned this pull request Mar 14, 2023
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.

3 participants