Skip to content

Commit

Permalink
Fix bug in translator replacements for strings that have the same beg…
Browse files Browse the repository at this point in the history
…inning. Fixes #1114.
  • Loading branch information
taylorotwell committed May 1, 2013
1 parent f1bb329 commit 2d35eef
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 0 deletions.
1 change: 1 addition & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
- Arrays returned from routes / controllers are now turned into JSON.
- Collection `map` and `filter` both return new `Collection` instances now.
- Added support for wildcard event listeners. For example, `Event::listen('view.*', function() {})`.
- Fix bug in translator replacements for strings that have the same beginning.

## Beta 4

Expand Down
17 changes: 17 additions & 0 deletions src/Illuminate/Translation/Translator.php
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<?php namespace Illuminate\Translation;

use Illuminate\Support\Collection;
use Illuminate\Support\NamespacedItemResolver;
use Symfony\Component\Translation\MessageSelector;
use Symfony\Component\Translation\TranslatorInterface;
Expand Down Expand Up @@ -109,6 +110,8 @@ protected function getLine($namespace, $group, $locale, $item, $replace)
*/
protected function makeReplacements($line, $replace)
{
$replace = $this->sortReplacements($replace);

foreach ($replace as $key => $value)
{
$line = str_replace(':'.$key, $value, $line);
Expand All @@ -117,6 +120,20 @@ protected function makeReplacements($line, $replace)
return $line;
}

/**
* Sort the replacements array.
*
* @param array $replace
* @return array
*/
protected function sortReplacements($replace)
{
return with(new Collection($replace))->sort(function($r)
{
return mb_strlen($r) * -1;
});
}

/**
* Get a translation according to an integer value.
*
Expand Down
9 changes: 9 additions & 0 deletions tests/Translation/TranslationTranslatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@ public function testGetMethodProperlyLoadsAndRetrievesItem()
}


public function testGetMethodProperlyLoadsAndRetrievesItemWithLongestReplacementsFirst()
{
$t = $this->getMock('Illuminate\Translation\Translator', null, array($this->getLoader(), 'en'));
$t->getLoader()->shouldReceive('load')->once()->with('en', 'bar', 'foo')->andReturn(array('foo' => 'foo', 'baz' => 'breeze :foo :foobar'));
$this->assertEquals('breeze bar taylor', $t->get('foo::bar.baz', array('foo' => 'bar', 'foobar' => 'taylor'), 'en'));
$this->assertEquals('foo', $t->get('foo::bar.foo'));
}


public function testGetMethodProperlyLoadsAndRetrievesItemForGlobalNamespace()
{
$t = $this->getMock('Illuminate\Translation\Translator', null, array($this->getLoader(), 'en'));
Expand Down

5 comments on commit 2d35eef

@Anahkiasen
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a simpler/faster way than that ? While we're still in dev I don't think backwards-compatibility matters that much.

@taylorotwell
Copy link
Member Author

@taylorotwell taylorotwell commented on 2d35eef May 1, 2013 via email

Choose a reason for hiding this comment

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

@Anahkiasen
Copy link
Contributor

Choose a reason for hiding this comment

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

But overall it isn't failproof, just a little less prone to errors, what I meant was while you're fixing the behavior, why not use a start and end delimiters so that it is actually completely secure ? And even if the sort is right I doubt it's faster than an str_replace.

@taylorotwell
Copy link
Member Author

@taylorotwell taylorotwell commented on 2d35eef May 1, 2013 via email

Choose a reason for hiding this comment

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

@Anahkiasen
Copy link
Contributor

Choose a reason for hiding this comment

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

Well if you have any variable where right next to it is a piece of string which would make it match another variable, it would fail. Like if you have two variables :foo and :foobar and want to write $foo. 'bar' it will replace it by :foobar.
I'm not saying this isn't an extreme case, but while we're in beta and that things that were a lot larger got changed, I don't see the cost of one character being worth the risk of having this code just waiting to fail on something a little more complex. Like if you had the opportunity to change Blade syntax to have an opening and a closing tag and avoid all the regex problems you've had since Laravel 3, then I'd say breaking compatibility is better than constantly fixing holes in the syntax – it's the same thing here. If it's better and more failproof, then people will adapt the new syntax, everything doesn't have to be backward compatible.

Please sign in to comment.