fixed attribution replacement #78
Conversation
if a translation is used as a link (localization)`<a href="email:*localizedEmail*">` the regex wouldn't match.
|
Thank you. Could you add a unit test to prove this regex? |
|
Sure thing! |
|
@Nyholm ok like this? |
Nyholm
left a comment
Nyholm
left a comment
There was a problem hiding this comment.
I think it looks good. Im not 100% sure about these classes though. So I would like a second review from an expert ;)
|
@Nyholm classes? |
damienalexandre
left a comment
damienalexandre
left a comment
There was a problem hiding this comment.
I agree with only one of the changes.
The one which remove attribute prefix looks strange to me, can you expose what is the issue with a <a href="mailto:🚫 Can't be translated here. 🚫"> link?
Tests/Functional/EditInPlaceTest.php
Outdated
| self::assertEquals(5, $xtrans->length); | ||
| // Check attribute with prefix (href="mailto:...") | ||
| $emailTag = $dom->getElementById('email'); | ||
| self::assertEquals('🚫 Can\'t be translated here. 🚫', $emailTag->getAttribute('href')); |
There was a problem hiding this comment.
That means the prefix is lost? Is that always a good idea?
From my opinion the prefix should not be removed (mailto:) - we only put the <x-trans> tag where is a translation and that should not interfere with the content around it.
There was a problem hiding this comment.
You're right. That's a bad idea. Already pushed the changes.
|
Ok now I'm ok with the change but the test is failing, looks like |
|
@damienalexandre That's strange. Locally the tests are green. I'l try to fix it.. but I have to commit/push each time. |
|
@damienalexandre Can you please look at the last two commits (TravisCI). This is strange. |
|
Yes it looks like there is a bug in PHP DomDocument / Travis I can't reproduce locally neither. I will investigate 👍 |
|
Ok I think it works with this: @$dom->loadHTML(mb_convert_encoding($response->getContent(), 'HTML-ENTITIES', 'UTF-8'));As you can see here: https://travis-ci.org/php-translation/symfony-bundle/builds/199987659 / 90b3c2c Could you update your branch accordingly? Thanks! |
|
Perfect, thanks a lot for your time and help! 👍 |
3 participants
<a href="email:*localizedEmail*">