search

bleroy / vandelay

Fine Latex Products - A module for Orchard CMS that does lots of stuff
13 stars 12 forks source link

TranslationManager: Ensure that doublequote characters are escaped in the output #47

Open BenedekFarkas opened 7 years ago

BenedekFarkas commented 7 years ago

Because it might fail to be parsed (it does happen in Crowdin), as discussed in https://github.com/OrchardCMS/Orchard/issues/7684. Although I'm not sure that applying a translated string would be able find its match, since they would be technically different in terms of the escaped vs. unescaped doublequotes. This is not really a problem in extension manifest files, since translation is not really meaningful there, but I wonder if there are other cases where not escaping a doublequote might be OK but cause problems in translations (C# and Razor files are covered by default syntactically, ofc, so we need to escape doublequotes anyway). What do you think?

bleroy commented 7 years ago

I have to admit I'm a little confused: either you found an actual buggy scenario, or not. I'm not sure what the actual issue is here.

BenedekFarkas commented 7 years ago

So the root of the problem is that Crowdin can't process the translation strings coming from extension manifests if they have an unescaped doublequote character (despite that those are syntactically correct). Sébastien suggested that extension manifests should keep this capability and they should be handled when generating the PO files. But: I assume that if we generate a PO file with the doublequotes escaped (and then you translate those strings with keeping the doublequotes escaped), translations won't match the original strings and your translated string won't be displayed (since msgid will be different). This is not really a problem for extension manifests, I guess, since the translations for those files are not really meaningful. Although it would be useful to modify TranslationManager to always make sure that any doublequote characer is escaped, so it doesn't need any manual work to be compatible with Crowdin.

bleroy commented 7 years ago

I see. Hey, going off topic for a second, but I'm wondering if it would make sense to fork this feature out of here, and into a new module that you could own, since you've been doing way more localization work than me lately. I could then obsolete this feature from Vandelay. Thoughts?

BenedekFarkas commented 7 years ago

Sorry, this slipped from my to-do list - sure, I'd be happy to take over maintaining this feature. Btw do you think it's possible/makes sense to refactor this in a way that instead of analyzing the code files, we'd use the compiler services to find the usages of Localizer?

bleroy commented 7 years ago

Yes, I think that's a great idea. We didn't have that option back then, but now it's the only thing that makes sense. Not sure how that'll play out with Razor files though.