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.

Sign up for GitHub

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

Feature/extract concat strings #379

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Feature/extract concat strings #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1f06b7a
Making it possible to extract text from concatenated strings
Apr 1, 2016
fa7bd90
Adding phtml files to scope of extraction
Apr 1, 2016
918cee9
Adding Tests to the new feature (extract from ConcatString)
Apr 4, 2016
71c6628
Adding the possibility of ignoring some annotations while extracting …
Apr 11, 2016
02ab8ad
Adding the option to ignore some annotations
Apr 11, 2016
487d6ec
removed ignored annotation part
Jul 29, 2016
adbdc3b
Removed extra linebreaks
Jul 29, 2016
d44483c
Removed extra tab
Jul 29, 2016
fd8a177
merging with original fork
Jul 29, 2016
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
9 changes: 9 additions & 0 deletions Tests/Translation/Extractor/File/DefaultPhpFileExtractorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ public function testExtractController()
$message->addSource(new FileSource($path, 82));
$expected->add($message);

$message = new Message('Foo Bar Moo');
$message->setDesc('Foo Bar Moo');
$message->addSource(new FileSource($path, 88));
$expected->add($message);

$this->assertEquals($expected, $catalogue);
}

Expand All @@ -80,6 +85,10 @@ public function testExtractTemplate()
$message->addSource(new FileSource($path, 3));
$expected->add($message);

$message = new Message('Foo Bar Moo');
$message->addSource(new FileSource($path, 5));
$expected->add($message);

$this->assertEquals($expected, $this->extract('template.html.php'));
}

Expand Down
6 changes: 6 additions & 0 deletions Tests/Translation/Extractor/File/Fixture/Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,10 @@ public function assignToVar()
/** @Desc("The var %foo% should be assigned.") */
return $this->translator->trans('text.var.assign', array('%foo%' => 'fooVar'));
}

public function concatString()
{
/** @Desc("Foo Bar Moo") */
return $this->translator->trans("Foo" . " Bar" . " Moo");
}
}
4 changes: 3 additions & 1 deletion Tests/Translation/Extractor/File/Fixture/template.html.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
<a href="#"><?php echo $view['translator']->trans('foo.bar') ?></a>

<a href="#"><?php echo /** @Desc("Foo Bar") */ $view['translator']->trans('baz', array(), 'moo') ?></a>
<a href="#"><?php echo /** @Desc("Foo Bar") */ $view['translator']->trans('baz', array(), 'moo') ?></a>

<a href="#"><?php echo $view['test']->trans('Foo' . ' Bar' . ' Moo') ?></a>
34 changes: 28 additions & 6 deletions Translation/Extractor/File/DefaultPhpFileExtractor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
use PhpParser\NodeTraverser;
use PhpParser\NodeVisitor;
use PhpParser\Node\Scalar\String_;
use PhpParser\Node\Expr\BinaryOp\Concat;
use Psr\Log\LoggerInterface;

/**
Expand Down Expand Up @@ -133,12 +134,12 @@ public function enterNode(Node $node)
}
}

if (!$node->args[0]->value instanceof String_) {
if (!$node->args[0]->value instanceof String_ and !$node->args[0]->value instanceof Concat) {
if ($ignore) {
return;
}

$message = sprintf('Can only extract the translation id from a scalar string, but got "%s". Please refactor your code to make it extractable, or add the doc comment /** @Ignore */ to this code element (in %s on line %d).', get_class($node->args[0]->value), $this->file, $node->args[0]->value->getLine());
$message = sprintf('Can only extract the translation id from a scalar string or a Concat, but got "%s". Please refactor your code to make it extractable, or add the doc comment /** @Ignore */ to this code element (in %s on line %d).', get_class($node->args[0]->value), $this->file, $node->args[0]->value->getLine());

if ($this->logger) {
$this->logger->error($message);
Expand All @@ -148,16 +149,20 @@ public function enterNode(Node $node)
throw new RuntimeException($message);
}

$id = $node->args[0]->value->value;
if ($node->args[0]->value instanceof Concat) {
$id = $this->concatToString($node->args[0]->value);
} else {
$id = $node->args[0]->value->value;
}

$index = $this->methodsToExtractFrom[strtolower($node->name)];
if (isset($node->args[$index])) {
if (!$node->args[$index]->value instanceof String_) {
if (!$node->args[$index]->value instanceof String_ and !$node->args[$index]->value instanceof Concat) {
if ($ignore) {
return;
}

$message = sprintf('Can only extract the translation domain from a scalar string, but got "%s". Please refactor your code to make it extractable, or add the doc comment /** @Ignore */ to this code element (in %s on line %d).', get_class($node->args[0]->value), $this->file, $node->args[0]->value->getLine());
$message = sprintf('Can only extract the translation domain from a scalar string or a Concat, but got "%s". Please refactor your code to make it extractable, or add the doc comment /** @Ignore */ to this code element (in %s on line %d).', get_class($node->args[0]->value), $this->file, $node->args[0]->value->getLine());

if ($this->logger) {
$this->logger->error($message);
Expand All @@ -167,7 +172,11 @@ public function enterNode(Node $node)
throw new RuntimeException($message);
}

$domain = $node->args[$index]->value->value;
if ($node->args[$index]->value instanceof Concat) {
$domain = $this->concatToString($node->args[$index]->value);
} else {
$domain = $node->args[$index]->value->value;
}
} else {
$domain = 'messages';
}
Expand All @@ -180,6 +189,19 @@ public function enterNode(Node $node)
$this->catalogue->add($message);
}

/**
* @param $node
* @return string
*/
private function concatToString($node)
{
if ($node instanceof String_) {
return $node->value;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider the following:
$translation->trans('foo'.$bar);

This will fail. You need to add tests for this and also check if it is an instance of Concat. If not we need to throw an Exception

return self::concatToString($node->left) . self::concatToString($node->right);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why a static call?

}
}

/**
* @param \SplFileInfo $file
* @param MessageCatalogue $catalogue
Expand Down
4 changes: 2 additions & 2 deletions Translation/Extractor/FileExtractor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ public function extract()
if (false !== $pos = strrpos($file, '.')) {
$extension = substr($file, $pos + 1);

if ('php' === $extension) {
if ('php' === $extension or 'phtml' === $extension) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

try {
$ast = $this->phpParser->parse(file_get_contents($file));
} catch (Error $ex) {
Expand All @@ -227,7 +227,7 @@ public function extract()
$visitingArgs[] = $ast;
} elseif ('twig' === $extension) {
$visitingMethod = 'visitTwigFile';
$visitingArgs[] = $this->twig->parse($this->twig->tokenize(file_get_contents($file), (string) $file));
$visitingArgs[] = $this->twig->parse($this->twig->tokenize(file_get_contents($file), (string)$file));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert this. It violates the code style.

Also, it is good practice not to make CS changes mixed with a feature. CS changes should be in a separate PR.

}
}

Expand Down