[Translation][Extractor] Allow extracting an array of files besides extracting a directory #14002
Conversation
f98d862 to
20d1b39
Compare
| $finder = new Finder(); | ||
| $files = $finder->files()->name('*.twig')->sortByName()->in($directory); | ||
| if (is_array($resource)) { | ||
| $files = array_filter($resource, function ($filename) { |
There was a problem hiding this comment.
$files = array();
foreach ($resource as $file) {
if (!is_file($file)) {
throw new \InvalidArgumentException(sprintf('The "%s" file does not exist.', $file));
}
if ('twig' === pathinfo($filename, PATHINFO_EXTENSION)) {
throw new \InvalidArgumentException('the given file type is not supported');
}
$files[] = new \SplFileInfo($file);
}There was a problem hiding this comment.
Some concerns about your comment:
- Isn't the
is_filecheck too expensive if the array contains a lot of files? - The second if statement is making this behave differently than before. Files with another extensions were ignored, no exception was thrown. I'd prefer to keep the old behaviour.
- Are you creating SplFileInfo instances only to keep the code from below the same?
There was a problem hiding this comment.
- Anyway we need to be sure that file exist, the Finder is expensive too
- ok
- Yes, it keep the code from below the same.
There was a problem hiding this comment.
Comments addressed. Also added more tests and different options for $resource like support for Traversable, a file, etc. Updated interface documentation too.
59790c3 to
4b5abd8
Compare
|
@marcosdsanchez what do you think about adding a new class that give ability to extract from one file (FileTwigExtractor for example) ? /cc @fabpot |
|
@aitboudad , what about something like 1b419cb ? |
|
@marcosdsanchez It's exactly what I want :D |
| /** | ||
| * @var string | ||
| */ | ||
| private static $extension = 'twig'; |
There was a problem hiding this comment.
we can avoid static here.
There was a problem hiding this comment.
Should it be a constant instead?
There was a problem hiding this comment.
0695b03 to
7185752
Compare
|
👍 |
| @@ -20,7 +21,7 @@ | |||
| * | |||
| * @author Michel Salib <michelsalib@hotmail.com> | |||
| */ | |||
| class PhpExtractor implements ExtractorInterface | |||
| class PhpExtractor extends AbstractFileExtractor implements ExtractorInterface | |||
There was a problem hiding this comment.
Does it make sense to let AbstractFileExtractor implement ExtractorInterface instead? Or can we think about a use case where this is not necessary?
There was a problem hiding this comment.
I thought about this too but decided to go this way as the abstract class doesn't implement any of the methods from the interface.
|
@aitboudad Because of the new class the code is not backwards compatible anymore. See https://travis-ci.org/symfony/symfony/builds/55525434 . What should I do? Get back to the version I had before or point the PR to another branch? |
|
just update composer.json to 2.7 |
|
@marcosdsanchez You will have to bump the version in the FrameworkBundle too. |
|
@aitboudad @xabbuh updated Twig Bridge and Framework Bundle. Travis is still failing with high dependencies in php 5.6 https://travis-ci.org/symfony/symfony/builds/55551337 . What am I missing? |
|
@marcosdsanchez This seems to be related to how that specific build works (you change is not available in the components repos as it was not merged). So that's nothing to worry about. |
|
Thanks @xabbuh . I think this is ready then. |
|
Can you please add an entry to the changelog? |
|
@xabbuh sure. For which version 2.3.27 (unreleased) ? |
|
@marcosdsanchez You need to add an entry in the CHANGELOG of the component, not the CHANGLOG file from the root directory (those are automatically generated.) |
|
@marcosdsanchez As @fabpot said, and the version should be 2.7.0. |
| /** | ||
| * @var string | ||
| */ | ||
| private $extension = 'twig'; |
There was a problem hiding this comment.
I would remove this as there is no way to change it and no need to change it.
There was a problem hiding this comment.
This was already discussed here. I'd prefer to have a class constant, but ok, I'll remove...
|
👍 when my 2 comments are fixed |
…a directory. The following commit adds the new feature to the following extractors: PhpExtractor and TwigExtractor. It also corrects the interface documentation to show that an array of files is also allowed as a parameter. A new base class called AbstractFileExtractor was created. It includes common code used by FileExtractors.
5258f01 to
4c5adbc
Compare
|
@fabpot comments addressed. |
|
Thank you @marcosdsanchez. |
…files besides extracting a directory (marcosdsanchez) This PR was squashed before being merged into the 2.7 branch (closes #14002). Discussion ---------- [Translation][Extractor] Allow extracting an array of files besides extracting a directory | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Give ability to extractors to extract from an array of files a file, or a Traversable besides a directory. The following commit adds the new feature to the following extractors: PhpExtractor and TwigExtractor. It also corrects the interface documentation to show the new options allowed as a parameter. I found this change useful as I've got a custom translation:update command that instead of extracting from $rootPath. '/Resources/views/' it extracts from the files that git is showing as modified. The command usually runs much faster than the default as it only parses the needed files. Commits ------- 9d6596c [Translation][Extractor] Allow extracting an array of files besides extracting a directory
Give ability to extractors to extract from an array of files a file, or a Traversable besides a directory.
The following commit adds the new feature to the following extractors:
PhpExtractor and TwigExtractor.
It also corrects the interface documentation to show the new options allowed as a parameter.
I found this change useful as I've got a custom translation:update command that instead of extracting from $rootPath. '/Resources/views/' it extracts from the files that git is showing as modified. The command usually runs much faster than the default as it only parses the needed files.