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

[#163] Added support for translation extractors for JS #238

Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Here Comes a New Challenger : FrontendExtractor
More flexible way to handle extraction from other sources than twig or php
jeremy-richard committed Feb 14, 2018
commit 0ee29c6953b5be8d1dd1625146290e402716cbca
4 changes: 4 additions & 0 deletions DependencyInjection/BazingaJsTranslationExtension.php
Original file line number Diff line number Diff line change
@@ -44,5 +44,9 @@ public function load(array $configs, ContainerBuilder $container)
->replaceArgument(4, $config['default_domain'])
->replaceArgument(5, $config['active_locales'])
->replaceArgument(6, $config['active_domains']);

$container
->getDefinition('bazinga.jstranslation.translation_frontend_extractor')
->replaceArgument(1, $config['frontend']);
}
}
11 changes: 11 additions & 0 deletions DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
@@ -34,6 +34,17 @@ public function getConfigTreeBuilder()
->prototype('scalar')
->end()
->end()
->arrayNode('frontend')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather name it extractor

->arrayPrototype()
->children()
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation should use 4 spaces

Copy link
Author

Choose a reason for hiding this comment

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

OK

->arrayNode('extensions')
->prototype('scalar')
->end()
->end()
->scalarNode('sequence')->end()
Copy link
Contributor

Choose a reason for hiding this comment

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

Forcing our users to configure the regex themselves looks like a bad DX to me. Maintaining this regex for the common cases should be the bundle responsibility (you can make it configurable in the extractor class to make it generic, but then, please provide pre-configured services)

Copy link
Author

Choose a reason for hiding this comment

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

Done. Hopefully the default configuration is alright.

->end()
->end()
->end()
->end();

return $builder;
13 changes: 0 additions & 13 deletions Extractor/CoffeeExtractor.php

This file was deleted.

27 changes: 20 additions & 7 deletions Extractor/Extractor.php → Extractor/FrontendExtractor.php
Original file line number Diff line number Diff line change
@@ -8,7 +8,7 @@
use Symfony\Component\Translation\MessageCatalogue;
use Symfony\Component\Translation\Extractor\ExtractorInterface;

abstract class Extractor extends AbstractFileExtractor implements ExtractorInterface
final class FrontendExtractor extends AbstractFileExtractor implements ExtractorInterface
{
const FUNCTION_STRING_ARGUMENT_REGEX = '\s?[\'"]([^"\'),]+)[\'"]\s?';
Copy link
Contributor

Choose a reason for hiding this comment

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

this should use possessive quantifiers to avoid useless backtracking, allowing to match on bigger files.

Copy link
Author

@jeremy-richard jeremy-richard Feb 14, 2018

Choose a reason for hiding this comment

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

I'm not familiar with possessive quantifiers. My regex skills aren't strong enough.
Is this one correct ?
\s*+['"]([^"'),]*+)['"]

Copy link
Contributor

Choose a reason for hiding this comment

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

yes (well, I would keep [^"'),]++ for the inner of the string, not allowing empty ones.

and btw, why forbidding commas and parenthesis inside the string ?

Copy link
Author

Choose a reason for hiding this comment

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

I'd say is to ensure the use of transalation key but the again I don't really know as I forked this feature where @jcchavezs left it.

Choose a reason for hiding this comment

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

That is exactly the reason but if this can be improved that is great. What I'd request @stof is to drop here the cases we are trying to avoid so they can be added to unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see 2 issues here:

  • forcing people to use translation keys to be able to use the extractor does not make sense. The extractor should not enforce such assumption.
  • there is nothing saying that these chars are forbidden when using translation keys. Translation keys can still be any string (the only thing about them is that you don't consider them as being also the English message, and so you don't edit the key when fixing a typo in your English message)

Copy link
Contributor

Choose a reason for hiding this comment

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

And here are a few cases:

t.trans('some,thing')
t.trans('display(details)'

Copy link
Contributor

Choose a reason for hiding this comment

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

The final space matching is useless (except to increase backtracking due to being non-possessive), as you don't care about what comes next

Copy link
Contributor

Choose a reason for hiding this comment

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

Spaces between the function call and the string should allow multiple spaces too, not just a single one.

const REGEX_DELIMITER = '/';
@@ -17,8 +17,11 @@ abstract class Extractor extends AbstractFileExtractor implements ExtractorInter

private $filesystem;

public function __construct(Filesystem $filesystem) {
$this->filesystem = $filesystem;;
private $configuration;

public function __construct(Filesystem $filesystem, array $configuration = []) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than an array of configurations here, with each configuration being a non-validated struct expected to have extensions and sequence key, I would rather have this extractor getting 2 constructor arguments: an array of extensions and a sequence. And then, if you need multiple sequences for different extensions, it would be a matter of registering several services using this class.

Copy link
Author

Choose a reason for hiding this comment

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

OK

$this->filesystem = $filesystem;
$this->configuration = $configuration;
}

/**
@@ -29,7 +32,14 @@ public function extract($resource, MessageCatalogue $catalogue)
$files = $this->extractFiles($resource);

foreach ($files as $file) {
$this->parseMessagesFromContent(file_get_contents($file), $catalogue);
foreach ($this->configuration as $configuration)
{
$pathInfo = pathinfo($file);
if (in_array($pathInfo['extension'], $configuration['extensions'], true))
{
$this->parseMessagesFromContent(file_get_contents($file), $catalogue, $configuration['sequence']);
Copy link
Contributor

Choose a reason for hiding this comment

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

there is nothing ensuring that $configuration['sequence'] will exist here (if you omit it from the bundle config file when defining the extractor, you won't get any configuration error, and you will get a notice here)

Copy link
Author

Choose a reason for hiding this comment

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

OK

}
}
}
}

@@ -41,9 +51,9 @@ public function setPrefix($prefix)
$this->prefix = $prefix;
}

private function parseMessagesFromContent($fileContent, MessageCatalogue $catalogue)
private function parseMessagesFromContent($fileContent, MessageCatalogue $catalogue, $sequence)
{
$messages = $this->getMessagesForSequence($fileContent, $this->sequence);
$messages = $this->getMessagesForSequence($fileContent, $sequence);

foreach ($messages as $message) {
$catalogue->set($message, $this->prefix.$message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting them always in the messages domain is wrong. The best solution would be to support extracting the domain properly (but this is hard). But the basic solution is at least to respect the default domain being configured (which may be changed to something else than messages which is very handy when your frontend needs less than 1% of all your translation messages)

Copy link
Author

Choose a reason for hiding this comment

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

Then should we specify the domain in the constructor? (as the TranslationDumper does)
You said we can use the default domain, do you know the way to retrieve it frome this part of the code?

Copy link
Contributor

Choose a reason for hiding this comment

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

The default domain should indeed be injected in the constructor.

@@ -53,7 +63,10 @@ private function parseMessagesFromContent($fileContent, MessageCatalogue $catalo
/**
* @return array
*/
abstract protected function getSupportedFileExtensions();
protected function getSupportedFileExtensions()
{
return call_user_func_array('array_merge', array_column($this->configuration, 'extensions'));
}

protected function canBeExtracted($file)
{
13 changes: 0 additions & 13 deletions Extractor/JsExtractor.php

This file was deleted.

9 changes: 3 additions & 6 deletions Resources/config/services.xml
Original file line number Diff line number Diff line change
@@ -26,13 +26,10 @@
<argument>%kernel.root_dir%</argument>
<tag name="console.command" command="bazinga:js-translation:dump" />
</service>
<service id="bazinga.jstranslation.translation_js_extractor" class="Bazinga\Bundle\JsTranslationBundle\Extractor\JsExtractor">
<service id="bazinga.jstranslation.translation_frontend_extractor" class="Bazinga\Bundle\JsTranslationBundle\Extractor\FrontendExtractor" public="true">
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be public. Extractors are totally fine as private services as Symfony does not retrieve them dynamically from the container. This would allow inlining the service.

Copy link
Author

Choose a reason for hiding this comment

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

OK

<argument type="service" id="filesystem"></argument>
<tag name="translation.extractor" alias="js" />
</service>
<service id="bazinga.jstranslation.translation_coffee_extractor" class="Bazinga\Bundle\JsTranslationBundle\Extractor\CoffeeExtractor">
<argument type="service" id="filesystem"></argument>
<tag name="translation.extractor" alias="coffee" />
<tag name="translation.extractor" alias="frontend" />
<argument></argument> <!-- frontend -->
</service>
</services>
</container>
19 changes: 0 additions & 19 deletions Tests/Extractor/CoffeeExtractorTest.php

This file was deleted.

Original file line number Diff line number Diff line change
@@ -2,20 +2,26 @@

namespace Bazinga\JsTranslationBundle\Tests\Extractor;

use PHPUnit\Framework\TestCase;
use Bazinga\Bundle\JsTranslationBundle\Extractor\FrontendExtractor;
use Bazinga\Bundle\JsTranslationBundle\Tests\WebTestCase;
use Symfony\Component\Translation\MessageCatalogue;
use Bazinga\Bundle\JsTranslationBundle\Extractor\Extractor;

abstract class AbstractExtractorTest extends TestCase
final class FrontendExtractorTest extends WebTestCase
{
const TEST_LOCALE = 'en';
const TEST_KEY_1 = 'test-key-1';

/**
* @var Extractor
* @var FrontendExtractor
*/
protected $extractor;
Copy link
Contributor

Choose a reason for hiding this comment

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

should be private

Copy link
Author

Choose a reason for hiding this comment

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

OK


public function setUp()
{
$container = $this->getContainer();
$this->extractor = $container->get('bazinga.jstranslation.translation_frontend_extractor');
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, this is the only place where you need the service to be public. But there is a much simpler way to handle all this: don't use a WebTestCase to test the FrontendExtractor but instantiate it directly in the test (the only other service it depends on is the Filesystem, which is dead easy to instantiate)

Copy link
Author

Choose a reason for hiding this comment

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

OK

}

/**
* @dataProvider resourcesWithNotValidTransFunctionUsage
*/
19 changes: 0 additions & 19 deletions Tests/Extractor/JsExtractorTest.php

This file was deleted.

3 changes: 3 additions & 0 deletions Tests/Fixtures/app/config/base_config.yml
Original file line number Diff line number Diff line change
@@ -19,3 +19,6 @@ bazinga_js_translation:
- messages
- numerics
- foo
frontend:
- { extensions: [ 'coffee' ], sequence: '\.trans(?:Choice)?\s' }
- { extensions: [ 'js', 'jsx', 'ts' ], sequence: '\.trans(?:Choice)?\(' }