Skip to content

Conversation

@mlocati
Copy link
Member

@mlocati mlocati commented Feb 18, 2016

I run a few tests again a PHP project with ~4300 translatable strings, comparing the results of xgettext and those of the PhpCode extractor.

I only found two problems:

  1. xgettext (correctly) does not extracts strings like the me in __($avoid['me']). I think that the correct solution would be to do not extract strings with variables (since it does not have any sense), but gettext stops as soon as it find a variable (for instance from __('Stop at the variable'.$var.'!') it extracts Stop at the variable) - so I'd adopt this same approach for consistency.

  2. Consider this example:

__(
  'line1'
 .' line2')

xgettext (correctly) extracts 'line1 line2', but the PhpCode extractor only extracts 'line1'.

I added some tests here to highlight the above problems.

@mlocati
Copy link
Member Author

mlocati commented Feb 18, 2016

Another problem I found is that the extractor fails in this case: __ ('string') (note the space after __).

@mlocati
Copy link
Member Author

mlocati commented Feb 18, 2016

  1. In https://github.com/mlocati/Gettext/commit/52400278e634d8f53fd586a00cbd2284596f7f70 I fixed the detection of open parenthesis after function names when we have spaces/comments in between
  2. In https://github.com/mlocati/Gettext/commit/f7708f2a12aa35fc7c8d0f6adf77045e6ad1c510 I enabled the detection of concatenated strings
  3. What do you think about the detection when we have variables inside the parenthesis (like __('Hi '.$name))?
    What shall we do?
    Take the string part before the variable names (like gettext) or skip the whole function?

@oscarotero
Copy link
Member

Maybe we should do the same as gettext (take the string part before the variable) to be fully compatible.

But, on the other hand, if the parameter contains variables, this is an anomaly and this may be notified (for example using an exception: "the id string in the file x, line x and column x contains a variable"). The problem is that depending of the function, the position of the arguments that can contain variables is different. For example this is fine:

__('hello %s', $name);

but this not:

p__('context', 'hello '.$name);

The problem to handle this is that the function extraction and manipulation are executed by different functions. So the easiest is to ignore the variables and do the same than gettext.

@mlocati
Copy link
Member Author

mlocati commented Feb 18, 2016

So the easiest is to ignore the variables and do the same than gettext.

Ok.
And ok also for this reason: let's assume that the app already has loaded a .mo file with the translations for 'Hi' and 'Hello', and we have this code:

switch ($form) {
    case 'short':
        $t  = 'Hi';
        break;
    case 'long':
        $t  = 'Hello';
        break;
}
$out = __($t);

This is totally legal (and is a very simplified version of what we have in our project).

So, I'll implement this behaviour.

@oscarotero
Copy link
Member

Good point 👍
Thank you very much.

@mlocati
Copy link
Member Author

mlocati commented Feb 19, 2016

Everything should be ok (at least in theory - I still have to perform some tests).

Travis is failing because it tests the merged version of my pull request, so the error should be fixed when #99 will be merged

@mlocati
Copy link
Member Author

mlocati commented Feb 19, 2016

If you relaunch https://travis-ci.org/oscarotero/Gettext/builds/110324540 it should pass now

@mlocati
Copy link
Member Author

mlocati commented Feb 19, 2016

I tested this PR against a project of mine (4300+ strings) and the output is the same as the one of xgettext now 😉
I think it's ready to be merged

oscarotero added a commit that referenced this pull request Feb 19, 2016
Extraction of variables and concatenated strings
@oscarotero oscarotero merged commit c6772b0 into php-gettext:master Feb 19, 2016
@oscarotero
Copy link
Member

Good job. Thank you!

@mlocati mlocati deleted the xgettext-edge-cases branch February 19, 2016 09:40
@mlocati
Copy link
Member Author

mlocati commented Feb 19, 2016

Good job. Thank you!

Thanks! It's a bit slower than before, but the results are now really good 😉

@oscarotero
Copy link
Member

Sensiolab reports this error https://insight.sensiolabs.com/projects/496dc2a6-43be-4046-a283-f8370239dd47/analyses/30
This code can be removed?

@mlocati
Copy link
Member Author

mlocati commented Feb 19, 2016

Whoops, it seems I left there that line I used for debug purposes...

@oscarotero
Copy link
Member

Ok, don't worry. I can remove it as soon I have my computer.

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