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

ease inheritance in order to support Twig/Timber #52

Open
drzraf opened this issue Jul 20, 2018 · 24 comments
Open

ease inheritance in order to support Twig/Timber #52

drzraf opened this issue Jul 20, 2018 · 24 comments

Comments

@drzraf
Copy link

drzraf commented Jul 20, 2018

It would be nice for wp-cli make-pot to support twig template out of the box.
Some of the building blocks:
timber/timber#1753
umpirsky/Twig-Gettext-Extractor#60

@swissspidy
Copy link
Member

I just read through these issues as well as timber/timber#1754 and https://timber.github.io/docs/guides/internationalization/.

Is Twig so popular in WordPress space and is Timber the de facto standard for it? Where would we draw the line when someone else comes with their templating engine and wants this included in wp i18n make-pot?

I am not really sure whether this is something WP-CLI should handle.

The Gettext library we use already supports Twig extraction:

https://github.com/oscarotero/Gettext/blob/eea87605224d25125a39a83fbc9bc1c28d16bb7a/src/Extractors/Twig.php
https://github.com/oscarotero/Gettext/blob/master/tests/assets/twig/input.php

If we can easily use that to make it work with Timber, I'm open for looking at it. Otherwise I'm not sure it's worth the effort.

Also, good to keep in mind that with this we'd need to scan (not necessarily load) every PHP file twice, once for good old gettext and once for Twig.

@drzraf
Copy link
Author

drzraf commented Jul 23, 2018

I'll try to offer a PR, but I hope bootstrap'ing WP is not a showstopper...
It's needed to:

  1. load twig and its filters
  2. load custom twig filters/functions from functions.php (if any)

@swissspidy
Copy link
Member

Personally I really don‘t want to load WP just because some projects might use Twig.

People should be able to use this command in environments where they need to create POT files on the fly, e.g. on Travis CI, the WordPress.org infrastructure, or in projects like https://github.com/wearerequired/traduttore

Perhaps a feature flag or a new sub-command could be used for this, but I feel like in that case this should be handled in a new command offered by Timber. That command can of course extend this one‘s code, so there shouldn‘t be much duplicate code.

@schlessera thoughts?

@drzraf
Copy link
Author

drzraf commented Jul 23, 2018

I fully understand the concern, but how to handle writing to one common PO file (in one run) while having two different bootstrap levels?

@swissspidy
Copy link
Member

You mean one common POT file?

Let's say your custom command is wp timber make-pot which creates a new file called twig.pot.

Then, you can run wp i18n make-pot --merge=twig.pot to create one single POT file containing all translations from Twig, regular PHP code and JavaScript code.

Of course you could combine these commands, e.g. wp timber make-pot && wp i18n make-pot --merge=twig.pot.

@drzraf
Copy link
Author

drzraf commented Jul 24, 2018 via email

@swissspidy
Copy link
Member

(I wondered whether make-pot --twig could have be run after_wp_load
while keep make-pot before_wp_load).

I wondered the same yesterday, but I'm not sure it's possible. Also, it could lead to unexpected results for users and would need thorough documentation.

It'd be annoying not being able to reuse the hundreds of useful lines already in
that package.

That's why I suggested extending this command. I bet it's possible to extend the MakePotCommand class in your own code.

Perhaps you'd need to require this package in composer and use WP_CLI::add_command( 'i18n make-pot', 'My_Extended_Makepot_Command' ); to override the existing one.

Either way, you wouldn't have to rewrite things from scratch.

I don't know the internals of WP-CLI that well yet, hence pinging Alain before to get his thoughts to figure out the best way of extension here.

@drzraf
Copy link
Author

drzraf commented Jul 24, 2018

Moving argument handling outside __invoke would ease reuse: class MakeTimberPot extends \WP_CLI\I18n\MakePotCommandstill seems a bit difficult because of __invoke magic;

@swissspidy
Copy link
Member

Makes sense. I‘ll try to have a look over the weekend. If you have a proposed solution in mind, feel free to create a PR :)

@drzraf
Copy link
Author

drzraf commented Jul 24, 2018

Thank you!

In the meantime, here is my working code for Twig extraction:

<?php

use Gettext\Translations;
use Gettext\Extractors\Twig;
use WP_CLI\I18n;

final class TwigExtractor extends Twig {
	use WP_CLI\I18n\IterableCodeExtractor;

	public static $options = [
                'twig'            => NULL,
		'extractComments' => [ 'translators', 'Translators' ],
		'constants'       => [],
		'functions'       => [
			'__'              => 'text_domain',
			'_e'              => 'text_domain',
			'_x'              => 'text_context_domain',
			'_ex'             => 'text_context_domain',
			'_n'              => 'single_plural_number_domain',
			'_nx'             => 'single_plural_number_context_domain',
			'_n_noop'         => 'single_plural_domain',
			'_nx_noop'        => 'single_plural_context_domain'
        ]
    ];

    public static function fromString($string, Translations $translations, array $options = []) {
        $options += static::$options;
        $twig = $options['twig'] ?: self::createTwig();
        $source = $twig->compileSource(new Twig_Source($string, ''));
        $functions = new I18n\PhpFunctionsScanner( $source );
        $functions->saveGettextFunctions( $translations, $options );
    }

    private static function createTwig() {
        $twig = new Twig_Environment(new Twig_Loader_Array(['' => '']));
        $twig->addExtension(new Twig_Extensions_Extension_I18n());
        $timber = new TimberTwig();
        $timber->add_timber_functions( $twig );
        $timber->add_timber_filters( $twig );
        return static::$options['twig'] = $twig;
    }
}

@drzraf
Copy link
Author

drzraf commented Jul 24, 2018

Could you also drop "final" from PhpFunctionsScanner? It's useful to inherit from it.

@schlessera
Copy link
Member

I don't think Twig or Timber is popular enough to warrant bundling functionality for them with standard WP-CLI, especially if it needs major changes to the way the command runs.

This is perfectly fine as a third-party command, though, and we can make sure that it is straight-forward enough to extend the bundled command to enable said third-party command.

Note: You can extend a bundled command with a third-party command and have that override the bundled command when installed through the package manager.

This could be something like this (untested):

class TwigAwareMakePotCommand extends MakePotCommand {
   public function __invoke( $args, $assoc_args ) {
      $twig = Utils\get_flag_value( $assoc_args, 'twig', false );

      // For Twig handling, defer execution to after WP was loaded.
      if ( $twig ) {
         WP_CLI::add_hook( 'after_wp_load', [ $this, 'process_twig' ] );
      }

      // Continue with bundled command execution.
      parent::__invoke( $args, $assoc_args );
   }
}

@drzraf
Copy link
Author

drzraf commented Jul 27, 2018

Timber is intended as a library/theme-dependency/... Not being a plugin, it currently can not bundle wp-cli commands. Renaming as per #62.

Problems with extending MakePotCommand as a wp-cli subcommand:

@drzraf drzraf changed the title wp-cli make-pot Twig (Timber) support ease inheritance in order to support Twig/Timber Jul 31, 2018
@swissspidy
Copy link
Member

phpdoc from MakePotCommand __invoke isn't inherited. {@inheritdoc} does not help.

Looking at https://make.wordpress.org/cli/handbook/commands-cookbook/#wp_cliadd_commands-third-args-parameter and https://github.com/wp-cli/wp-cli/blob/b00cdfa4fcbe54a352ce7bc1431c3fe47989d2ae/php/WP_CLI/Dispatcher/CommandFactory.php it seems like it should be possible to get the doc block from MakePotCommand via reflection and pass the parsed doc block as a third argument to WP_CLI::add_command()

Impossible to extend from subcommand: Either __invoke is used and @subcommand foo is not taken into account, either __invoke is made no-op but subcommands are not even registered.

I'm not sure I can follow. Could you perhaps share some example code so I can better understand what's not working?

@drzraf
Copy link
Author

drzraf commented Jul 31, 2018

<?php
// functions.php: if ( defined( 'WP_CLI' ) && WP_CLI ) WP_CLI::add_command( 'timber', 'My_Timber' );

class My_Timber extends \WP_CLI\I18n\MakePotCommand {
	/**
	 * {@inheritdoc}
	 *
	 * [--twig]
	 * : Do additional Twig extraction before saving Po file
	 */
	public function __invoke( $args, $assoc_args ) { // attempt 1
		// error: option definition is wrong, inheritdoc won't work, and even less with intend of synopsis extension.
	}

	/**
	 * Create a po file ... 
	 *
	 * ## OPTIONS
	 *
	 * <source>
	 * : Directory to scan for string extraction.
	 * [...] full synopsis copy/paste from MakePotCommand
	 *
	 * ## EXAMPLES
	 *     $ wp timber make-pot . languages/my-plugin.pot

	 * @subcommand make-pot
	 * @when init
	 */
	public function __invoke( $args, $assoc_args ) { // attempt 2
		/* - With `wp timber make-pot . languages/my-plugin.pot`
		   error: Too many positional arguments
		   Because __invoke() is not even called. [make-pot is not a correctly registered command
		   - Only `wp timber . languages/my-plugin.pot`
		   is accepted, which is undesirable */
	}

	public function __invoke( $args, $assoc_args ) { // attempt 3
		// Make it a no-op, but define a subcommand below...
	}

	/**
	 * Create a po file ... 
	 */
	public function my_make_pot( $args, $assoc_args ) {
		// never registered, because __invoke() is a no-op.
	}
}


class My_Timber extends \WP_CLI_Command { // attempt 4
	/**
	 * Create a po file ... 
	 *
	 * [full synopsis, possibly with additional options, like --twig]
	 *
	 * @subcommand make-pot
	 * @when init
	 */
	public function my_make_pot( $args, $assoc_args ) {
		$twig = \WP_CLI\Utils\get_flag_value( $assoc_args, 'twig', false );
		// Continue with bundled command execution.
		$main_pot = new \WP_CLI\I18n\MakePotCommand();
		$main_pot->handle_arguments( $args, $assoc_args );
		// we would like to access $source, $destination, $translations, as processed by MakePotCommand, this is not currently possible
		// we would like to append to $main_pot->translations before storing the file, this is not currently possible (#63)
		$this->makeTwigPot();

	}
}

@schlessera
Copy link
Member

@drzraf Small note regarding the above code: The idea of overriding the existing command was to add the --twig flag to the existing command. For this to work, you should register the command with the same name as the one you want to extend, so that your extension takes precedence and only one is loaded:

WP_CLI::add_command( 'i18n make-pot', 'TwigAwareMakePotCommand', [array with options] );

I don't know yet how best to handle the synopsis, I'll have to experiment with that myself. But know that you can also define the synopsis through the [array with options] in the WP_CLI::add_command() call: https://make.wordpress.org/cli/handbook/commands-cookbook/#wp_cliadd_commands-third-args-parameter

@swissspidy
Copy link
Member

If you instead want to make the command be named wp timber make-pot I'd do something like this:

WP_CLI::add_command( 'timber make-pot', 'My_Timber' );

class My_Timber {
    public function __invoke() { /* ... */ }
}

See https://make.wordpress.org/cli/handbook/commands-cookbook/#required-registration-arguments for details about the magic method __invoke() and how WP-CLI handles public methods.

@drzraf
Copy link
Author

drzraf commented Jul 31, 2018

@swissspidy I could inherit the way you suggested (but still copying the full synopsis).

Then I tried synopsis "inheritance". Command creation accept longdesc (string), which docparser provides.
But it can't be overriden afterward synopsis would completely override longdesc from docparser, so I'm bound to inline dirty preg/replace. @schlessera ?

    $reflection = new \ReflectionClass( "WP_CLI\I18n\MakePotCommand" );
    $docparser = new \WP_CLI\DocParser( $reflection->getMethod( '__invoke' )->getDocComment() );
    $ld = explode("\n", $docparser->get_longdesc());
    $new_ld = '';
    $isopt = 0;
    foreach($ld as $l) {
        if($isopt==1 && strpos($l, '##') === 0) {
            $new_ld .= <<<EOF
[--twig]
: With Twig
EOF
                    . PHP_EOL . PHP_EOL;
        }
        if($l=='## OPTIONS') $isopt=1;
        $new_ld .= $l . PHP_EOL;
    }
    $args = ['shortdesc' => $docparser->get_shortdesc(), 
             'longdesc' => $new_ld,
             'synopsis' => $docparser->get_synopsis(), 
             'when' => 'init' /*$docparser->get_tag('when')*/ ];
    WP_CLI::add_command( 'timber make-pot', 'My_Timber', $args);

Providing a couple of helpers to \WP_CLI\DocParser could be nice, eg: addOption(), setExample(), ... or maybe map DocParser components to Symfony Console/InputOptions :)

@schlessera
Copy link
Member

@drzraf Yes, I agree, this should be easier to do, and methods for WP_CLI\DocParser seem like a good addition.

I'll accept PRs for that if you care to provide them.

@swissspidy
Copy link
Member

Is there anything left here for the WP-CLI / i18-command side of things?

@drzraf
Copy link
Author

drzraf commented Oct 28, 2018

Chronologically:

  1. Need to support Timber Twig templates extraction (¹) => Answered: Twig isn't mainstream enough.
  2. But there is no easy way to call an out-of-tree extractor from vanilla make-pot.
  3. => Try overriding the whole i18n make-pot command => difficult and unlikely to work from a plugin/theme.
  4. => Create a custom command that wraps i18n make-pot (called timber make-pot) => ok
  5. But still needs full synopsis copy/pasting inside the wrapped command
  6. => Above ease inheritance in order to support Twig/Timber #52 (comment) about a helper that let me just append an argument to (inherited) make-pot initial synopsis.

These actually are ... a big stack of painful workarounds.
There is definitely things that could be done either on wp-cli core side, either on i18n-command side. Naming a few:

  1. Support Twig extraction out of the box in i18n-command
  2. Add some action/filter to MakePotCommand::extract_strings to allow custom extractors (+ corresponding argument handling)
  3. Ease MakePotCommand class inheritance to allow arguments handling and extract_strings override.
  4. Allow command inheritance by allowing option/arguments manipulation from PHP code. One way would be providing some DocParser helpers to change synopsis (like the above simili-addCommandOption())

¹ In the same POT file.

@xavivars
Copy link

Not sure I completely understood all issues in here. It's probably true that Twig/Timber is not used enough to deserve being part of the wp-cli core packages, but I think Twig it's used enough (it's the de facto PHP template language in frameworks like symphony or CMS like Drupal) that wp-cli supports enough so it can be extended to leverage Twig extraction (maybe Timber specifics could come later).

@drzraf
Copy link
Author

drzraf commented Feb 17, 2019

About Twig: Timber is the implementation of Twig for WordPress (function binding/gettext wrapping/helper about templating and ACF/...)

About string extraction, you may want to look at this stack:

@nlemoine
Copy link

nlemoine commented Jan 24, 2024

Hello there,

This feature has been requested since quite a long time. I'm happy to announce that we finally manage to handle WordPress translations extraction from Twig file using WP-CLI:

https://github.com/timber/wp-i18n-twig

Technical details (WP-CLI)

In order to provide a consistent DX, the command overrides the default make-pot command. That required a couple a hacky things:

Just mentioning those details in order to improve extending WP-CLI commands in the future, if that's something you might consider.

Technical details (Twig)

Although it's released by the Timber team, the package is Timber agnostic, should be working with any Twig implementation (see limitations) and outside the project if needed (CI, etc.).

The key here is to avoid compiling Twig files (which would lead to many problems), the logic has been borrowed from Symfony Translation extractor and consists in only parsing Twig templates and adding a "visitor" that collects all translations found in nodes.

The collected translation function calls are shaped as an array just like what the one expected by the gettext PhpFunctionsScanner class.

Sadly, Twig comments are completely skipped by the lexer and can't be found in the parsed tokens. Translator comments extraction was made possible at the cost of really bad code practice 🙈 but I felt it was still better than writing/overriding/maintaining a whole Lexer class just to grab comments that were already available and "easy" to be picked up here.

The benefit is that the package covers 100% of wp i18n tests 🥳

Twig and WP-CLI

Since wp i18n is also supporting Blade templates, would you be willing to add Twig support? I have of course seen the previous discussions/issues about this. I know that a third package is preferred. I'm just wondering if, considering this new package covers all tests, you might reconsider (I could add Twig 2.0 support if needed, which requires "only" PHP ^7.0).

I would obviously understand if you're not, this would ring another scope to maintain, etc. I'm fine keeping it as a third party package 😄

@schlessera @swissspidy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants