processwire / processwire-requests

ProcessWire feature requests.
39 stars 0 forks source link

Adding extension html for translatable files inside findTranslatableFiles() method, so that html files will be found (solution included!!) #480

Closed juergenweb closed 1 year ago

juergenweb commented 1 year ago

Short description of the enhancement

Include files with extension html in the translatable files, not only php, module and inc.

Current vs. suggested behavior

By default, only files with the extension php, module or inc will be found to be translatable. I am using html files with translatable strings too, but they will not be found until you enter the path manually at the bottom of the "Select file(s)" page in the backend.

Why would the enhancement be useful to users?

I use HTML files with translatable strings for my email templates and if I want to translate them, I had to add the files manually by adding the path to the files at the bottom inside the text input for "Enter file to translate". This assumes that you know, that there are translatable strings inside the HTML files and requires and extra step. If you are using HTML files with translatable strings inside a module, like in my case, people who are using this module will not know, that there are HTML files with translatable strings inside. This leads to, that they do not enter the files manually and the email texts will not be translated. Adding HTML files by default will solve this issue.

Simple implementation by adding only some strings

To allow files with html extension, you only need to go to line 817 of ProcessLanguageTranslator.module and replace the following line

if($ext != 'php' && $ext != 'module' && $ext != 'inc') continue;

with the following

if($ext != 'php' && $ext != 'module' && $ext != 'inc' && $ext != 'html') continue;

This has no negative side effect and will help to find translatable strings in html files by default

teppokoivula commented 1 year ago

Wondering if this should be a configurable thing, so that users could specify custom extensions when needed? For an example I would imagine that some might want .twig etc. to be included.

It's not entirely true that there are no negative side effects. This would make ProcessLanguageTranslator process every .html file it finds, which of course adds overhead. Depends a lot on case at hand how much, e.g. how many of such files there are.

juergenweb commented 1 year ago

Hello Teppo! Thanks for your response.

It's not entirely true that there are no negative side effects. This would make ProcessLanguageTranslator process every .html file it finds, which of course adds overhead. Depends a lot on case at hand how much, e.g. how many of such files there are.

You are right, but... I am convinced, that the amount of HTML files will not be as high as it can have an impact on performance. You have the same problem if you have a very large amount of PHP files that should be processed. Isn't it? It seems that most of the modules do not use HTML files at all, but you are right... if the amount of HTML files on a site is high, it could lead to an overhead and will have an impact on performance.

Wondering if this should be a configurable thing, so that users could specify custom extensions when needed? For an example I would imagine that some might want .twig etc. to be included.

Configurable would be the best option, because you are more flexible. :-)

Another possibility: Instead of adding the html extension by default to the list of extensions, I would be satisfied if there will be a hookable method to add it on demand. Unfortunately I have not found one, thats why I have suggested to add it directly to the ProcessLanguageTranslator.module file.

BernhardBaumrock commented 1 year ago

I'm confused. How would a translatable string look like in an HTML file??

<h1><?= __('Some great news') ?></h1>

That makes no sense to me. This would be a PHP file after all, no?

Though having translatable strings in LATTE files would make a lot of sense! Not sure what would be necessary to support that. Or if that can even work?!

<h1>{__('Some great news')}</h1>

So I agree with @teppokoivula that the extension should be configurable. And I'd be happy to get translatable strings in LATTE/TWIG :)

teppokoivula commented 1 year ago

I am convinced, that the amount of HTML files will not be as high as it can have an impact on performance. You have the same problem if you have a very large amount of PHP files that should be processed. Isn't it?

This is precisely why I mentioned performance. I've run into cases where current behaviour essentially locked the site for a considerable time, and sometimes timed out entirely. Admittedly this was related to a situation where directories containing a lot of data were involved :)

(A large variety of tools were symlinked so that they would be easily available if needed, which in turn caused a bit of a bottleneck here.)

It seems that most of the modules do not use HTML files at all, but you are right... if the amount of HTML files on a site is high, it could lead to an overhead and will have an impact on performance.

I've never heard of anyone needing translations in HTML files before. Doesn't mean that such a thing couldn't be supported, but I believe it is an exceedingly rare use case.

I would lean towards configurability, and not having too many extensions included by default.

juergenweb commented 1 year ago

I've never heard of anyone needing translations in HTML files before. Doesn't mean that such a thing couldn't be supported, but I believe it is an exceedingly rare use case

I agree with you. My use case is that I have written a module, which sends HTML emails and this email files contain the HTML markup and of course -> text. This text contains translatable strings, and this is the use case and the reason, why I have written this request issue.

Now, I am probably going the way @BernhardBaumrock suggested to me: Instead of using HTML email templates I will add a HTML editor field (multi-language) for each email, so the user can adapt the text easily according to his needs. In this case I do not need html files to be supported by the LanguageTranslator module.

But it would be great if LATTE or TWIG files would be supported. So a configurable solution would do the trick.

ryancramerdesign commented 1 year ago

I'm not sure I understand because __('text'); is a PHP function and will only work in a file that is executed as PHP. So maybe you could put that in a non-PHP file to make it translatable, but there would be no way to output the translation from that file without PHP. Or is there something I'm missing? :)

BernhardBaumrock commented 1 year ago

@ryancramerdesign yeah that's what I wrote yesterday :) But maybe you can think of a solution to the problem of translations when using a template engine like .latte or .twig?

My workaround is described here: https://github.com/baumrock/RockFrontend#latte-and-translatable-strings

I just tried what @juergenweb mentioned... In the latte file I had to use the namespaced version of () like this: `

{\ProcessWire\('deutsch')}
`

Then I added that file manually to the translation backend and wow - the string showed up in the backend! I translated that string to "german" and saved it.

Then I went back to the frontend and saw: deutsch - both in the DE and EN version of the website. So that's basically what @juergenweb described in his original post. The translation does not work.

Sorry for hijacking that issue, but I think that's the bigger picture of the problem :) And while @juergenweb did refactor his approach anyhow I think it would be great to have translatable strings in non-php files like latte or twig!

Maybe you have an idea how we could approach that? Thx in advance!

ryancramerdesign commented 1 year ago

@BernhardBaumrock I see, thanks, yeah I think we could make the extensions configurable. But as for how someone makes some file format run the PHP __() function call, I think we have to leave that up to the person. Sounds like that Latte can run PHP functions somehow.

BernhardBaumrock commented 1 year ago

Yeah latte compiles the .latte file to a .php file which makes it extremely fast :)

This is the cached latte-php file:

<?php

use Latte\Runtime as LR;

/** source: /var/www/html/site/templates/sections/main.latte */
final class Template14e122ba3c extends Latte\Runtime\Template
{

    public function main(array $ʟ_args): void
    {
        extract($ʟ_args);
        unset($ʟ_args);

        echo '<main ';
        echo LR\Filters::escapeHtmlTag(alfred()) /* line 1 */;
        echo '>
  <div class=uk-padding>
    ';
        echo LR\Filters::escapeHtmlText(\ProcessWire\__('test')) /* line 3 */;
        echo '
  </div>
  ';
        echo LR\Filters::escapeHtmlText($rockpagebuilder->render(true)) /* line 5 */;
        echo '
</main>
';
    }
}

Does that make any sense to you or in other words do you see a chance here to get translations working in such files?

ryancramerdesign commented 1 year ago

@BernhardBaumrock For that Latte file, I'm still not sure I understand how it works because translations are built around a Textdomain which is based on the php filename that outputs it. ProcessWire figures out the Textdomain automatically from a debug backtrace to the calling PHP file. So it would be looking for translations in the compiled .PHP file rather than the original Latte file. So as I understand it technically a translation for __('test'); would just output "test" rather than whatever was translated.

juergenweb commented 1 year ago

Just an idea:

Adding a small method for the extensions, that is hookable fe

___addExt()

This method returns fe ['module, 'php'] - the default extensions as array and can be extended with a Hook to add further extensiontypes:

$this->addHookAfter('ProcessLanguageTranslator::addExt', $this, 'methodToAddfurtherExtensions');

The after Hook fe returns ['module, 'php', 'latte', 'twig', 'whatever'].

This could be further used at line 817 of ProcessLanguageTranslator.module to replace this line

if($ext != 'php' && $ext != 'module' && $ext != 'inc') continue;

with something similar, that uses the array values above.

if($ext != the array values) continue;

This could be a possiblity to add extensions on demand.

BernhardBaumrock commented 1 year ago

NICE one Ryan, thx, I got it working!! 🥳

What I did:

That means:

I understand what's going on and why it is not working out of the box. Latte grabs the file main.latte and creates a cached/compiled PHP file on demand. That file will be stored in site/assets/cache/Latte/templates-sections-main.latte--14e122ba3c.php and that is the file that PW sees in the backtrace. So PW will look for translations of that compiled file rather than for the original .latte file and will obviously not find a translation and always show the default value!

Translating the compiled file in the backend is a proof of concept but for sure not a solution, as the filename always changes with some kind of hash...

Idea/Request:

As far as I understand we need two changes to make translations work in latte files (I don't know if the same would apply to Twig):

  1. Make the file extension for translatable files configurable
  2. Make ProcessWire skip all files in site/assets/cache/Latte from the backtrace.

@1

Ideally that would be some kind of API call like this, that I can do in RockFrontend.module.php:

public function init() {
  $this->wire->languages->addTranslatableExtension('latte');
}

@2

That's something I do in RockFrontend already, but not for translations. As far as I understand from your explanation PW sees the translatable string during rendering. It then checks via backtrace where that string was defined. Usually that shows something like /site/templates/home.php but when using latte, it will show something like /site/assets/cache/Latte/... instead of /site/templates/whatsoever.latte. If the latte cache folder was skipped here the returned filepath of the translatable string should correctly return /site/templates/whatever.latte and therefore it should find the translation from the backend!

I think that should work, right?

The only thing missing would be to make latte files understand {__('test'} instead of {\ProcessWire\__('test')}

ryancramerdesign commented 1 year ago

@BernhardBaumrock PW only has to figure out the Textdomain if you don't specify one. So for this case, I would specify the textdomain yourself as the second argument to the __() function call, i.e.

__('test', '/site/templates/whatever.latte'); 
BernhardBaumrock commented 1 year ago

@ryancramerdesign didn't know about that one!! I've ALMOST got a solution!!

The only problem left now is that PW does not recognise translatable strings in my latte file:

ProcessLanguageTranslator: That file has no translatable phrases

This is the content of the file:

<div>{__('foo')}</div>
<div n:syntax="double">{{__('bar')}}</div>

If I do {\ProcessWire\__('foo')} instead it works, but I'd REALLY prefer the short version without the namespaces. Could the translator please be updated with 2 changes:

1) make it accept custom extensions like .latte or .twig 2) make it accept translatable strings in latte file with latte syntax like {__('translatable string'} or {{__('translatable string'}}

That would be huge, thx! :)

Update: It works using this syntax: {$rockfrontend->_('translatable string')} so I'd be fine with that! Only part missing is (1) which is what @juergenweb initially requested 😅

ryancramerdesign commented 1 year ago

@BernhardBaumrock I still don't know how that all works in terms of the textdomains, but glad to hear it works. I think the reason that {__('translatable string')} doesn't work is because it wouldn't appear like that in PHP. If you could do { __('translatable string') }(with the spaces) or {(__('translatable string'))} (with extra parenthesis) that might possibly work. Though glad to hear {$rockfrontend->_('translatable string')} works. Note that in that case, it will be using the textdomain of whatever is the class of the $rockfrontend variable.

Note sure if this helps, but this is what I do with my front-end translations that might be needed in another file or multiple files, so that I can reuse them in any template files and only have to translate them once (in /site/templates/classes/Text.php):

<?php namespace ProcessWire;
/**
 * @property string $helloWorld
 * @property string $foo
 * @property string $bar 
 */
class Text extends Wire {
  public function __get($key) {
    switch($key) {
      case 'helloWorld': return $this->_('Hello World');
      case 'foo': return $this->_('Foo');
      case 'bar': return $this->_('Bar'); 
    }
    $value = parent::__get($key);
    return $value === null ? $key : $value; 
  }
}

Then in /site/templates/_init.php

require_once('./classes/Text.php'); 
$text = new Text();

and in any other template files...

echo "<p>$text->helloWorld</p>";

So I wonder if something like this would work in those Latte files, passing in that $text variable that has access to any translations? This way you'd still be translating a PHP file, but use the translations in any latte files. I can add new supported extensions either way though.

ryancramerdesign commented 1 year ago

@juergenweb @BernhardBaumrock I have updated the ProcessLanguageTranslator to make file extensions a configuration setting with the module.

BernhardBaumrock commented 1 year ago

OUTDATED! PLEASE READ THE UPDATES IN THE NEXT POST

Hey @ryancramerdesign thx for the update! Unfortunately that does not work on my end!

I've added latte to the list of allowed extensions: image

But even if I hit "refresh file list" it does not show up: image

I even tried to add a latte file directly in /site/templates (not in the sections subfolder) but that didn't work as well.

Only if I add the file manually I can translate it: image

Then the translation already works with the latest version of RockFrontend 😎

This is the code in the LATTE file:

<main {alfred()}>
  {$rockfrontend->_('Das ist ein Test')}
  {$rockpagebuilder->render(true)}
</main>

And FYI this is the method that takes care of finding the correct textdomain automatically: https://github.com/baumrock/RockFrontend/blob/8ebd80cdeda70c75835148fa7d619fc4cf31ae81/RockFrontend.module.php#L211-L238

Thx for explaining your approach! That's similar to what I did/recommended until now with RockFrontend: https://github.com/baumrock/RockFrontend/tree/8ebd80cdeda70c75835148fa7d619fc4cf31ae81#latte-and-translatable-strings

But it's definitely nicer to be able to translate directly in the file without having to first define a variable/key somewhere else. Of course that has other benefits, but that's another story :)

BernhardBaumrock commented 1 year ago

Update @ryancramerdesign

Turns out that when using '{ __('Das ist ein Test') }` (with spaces) it finds the file to translate!

image

Unfortunately latte does not support tags with spaces: image

But I just tried and it finds the latte files properly if I use {$rockfrontend->__('Das ist ein Test')} (with double underscore).

Do you think that is something that can be changed easily? If not I'm happy with the current solution :)

BernhardBaumrock commented 1 year ago

Another update @ryancramerdesign !

I've updated RockFrontend to include the following file to support latte translations like this: {__('Das ist ein Test')}

<?php // no namespace!
function __($str)
{
  return "Translated $str"; // TBD
}

That makes the translation function globally available, also within compiled latte-php files that don't have the ProcessWire namespace. So far so good.

Turns out that the file is found in the backend: image

But in the translation GUI it does not find that string any more! image

ryancramerdesign commented 1 year ago

@BernhardBaumrock I think that {__('Das ist ein Test')} can be supported by updating this line: https://github.com/processwire/processwire/blob/dev/wire/modules/LanguageSupport/LanguageParser.php#L231 which contains this:

'/([\s.=(\\\\,]__|=>__|^__)\(\s*' . // __(

and I think it would need the { added to the first character matching set (which currently matches whitespace, period, equals, open-paren, backslash or comma), like this, so that it also matches an opening curly brace:

'/([\s.=(\\\\,{]__|=>__|^__)\(\s*' . // __(

I don't think a } would need to be added to the regex, since it does a "match all" at the end. Are you able to check if that enables it to be translatable? If so, we'd need to update a couple other regex's too (the _n() and _x() ones), but figured the __() would be a good test to check.

BernhardBaumrock commented 1 year ago

Hey @ryancramerdesign thx for having a look! Turned out it was not as easy as you suggested, because the regex in https://github.com/processwire/processwire/blob/91f4b7cd6ee7eff6cc5b551b103c6d85fa50e2a0/wire/modules/LanguageSupport/LanguageParser.php#L219 did already not find it.

But the good news is that I found a way that works for all situations!! We just have to use {=__('my string')} and I think that's perfectly fine and better than messing around with complex regexes that might lead to trouble :)

I've already pushed the changes and in my opinion we can close that issue @juergenweb

Thx Ryan, really happy about that update!!! :)