Skip to content

Conversation

@toooni
Copy link
Contributor

@toooni toooni commented Mar 7, 2017

makes use of php-translation/extractor twig2 support

@Nyholm Nyholm self-requested a review March 7, 2017 14:13
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you. I like this change.

What do you think about taking a different approach to avoid using container parameters?

Maybe create config/extractors_twig1.yml and config/extractors_twig2.yml? Then one can do something like this in the TranslationExtension:

if (\Twig_Environment::MAJOR_VERSION === 1) {
  $loader->load('extractors_twig1.yml');
} else {
  $loader->load('extractors_twig2.yml');
}

# Twig Visitors:
php_translation.extractor.twig.visitor.translation_block:
class: Translation\Extractor\Visitor\Twig\TranslationBlock
class: %php_translation.extractor.twig.visitor.translation_block.class%
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to quote this parameter


php_translation.extractor.twig.visitor.translation_filter:
class: Translation\Extractor\Visitor\Twig\TranslationFilter
class: %php_translation.extractor.twig.visitor.translation_filter.class%
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to quote this parameter

@toooni
Copy link
Contributor Author

toooni commented Mar 7, 2017

@Nyholm Having two separate files could lead to problems with autocompletion (PHPStorm might lead to the wrong file)? But I am not sure what is better.
You can tell me what to do. It's not very hard to change the PR.

@Nyholm
Copy link
Member

Nyholm commented Mar 7, 2017

PHPStorm does not use Yaml files to autocomplete services. It is using the var/cache/dev/appDevDebugProjectContainer.php. That file is built on cache warmup. So we will not have any issues with multiple files.

There is no big deal and we could change this in a patch release later if we want to.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you!

$container->getDefinition('php_translation.extractor.php.visitor.FormTypeChoices')
->addMethodCall('setSymfonyMajorVersion', [Kernel::MAJOR_VERSION]);

if (\Twig_Environment::MAJOR_VERSION === 1) {
Copy link
Member

Choose a reason for hiding this comment

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

This variable was introduced in 1.28.0. We should use version_compare and \Twig_Environment::VERSION instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nyholm done

@toooni
Copy link
Contributor Author

toooni commented Mar 7, 2017

@Nyholm you're right about the autocompletion. Another thing then would be the duplication of all other services in extractors.yml. A solution would be splitting extractors.yml into extractory_twig.yml and extractors_php.yml or something.

@Nyholm
Copy link
Member

Nyholm commented Mar 7, 2017

Yes, I agree..
Let's solve that later =) I think this is good for now.

@Nyholm Nyholm merged commit a8adc4f into php-translation:master Mar 7, 2017
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

Successfully merging this pull request may close these issues.

2 participants