Port potx to Drupal 8

See #2356469: Extracting Drupal 8 translations with POTX for now.

So, I was gone for a while from potx development, but I was still following the issues and comments on potx. And, the main requested feature has been porting potx to Drupal 8. So, let's start from here.

There has also been a project on https://github.com/kgaut/drupal-potx/ in the meantime, maintained by @Kgaut. I have looked at the code, and will try to merge that code as much as possible back here, and credit the relevant users.

As a start, I have made a patch from the Drupal 8 port commits on the github project, plus a small change in the potx.install file. This should get the potx drush command working on Drupal 8.

Next, I will port the potx.module and the potx admin code to Drupal 8, and create the 8.x-1.x branch.

Yay thanks for working on this:

+++ b/potx.info.yml
@@ -0,0 +1,8 @@
+dependencies:
+  - locale

Does it depend on locale in Drupal 8? Not just as an optional dependency (for translation extraction?). I would say it does not depend on locale. (OTOH it would not depend with the same logic on it either in Drupal 7, hah)

Posting a progress patch.
I've ported the Potx admin UI and methods in this patch. Also, opened the 8.x-1.x branch to start testing the patches.

Next, I'll port the simpletest tests.

Does it depend on locale in Drupal 8?

Once the tests are green, I can remove the locale dependency, and see what happens.

Test was sent to D7. Let's try again.

Ported the tests to Drupal 8. Moved the test files around, removed t() usage from assert messages, and fixed a test that failed on newer PHP versions (which also fails on Potx D7; Will post a patch for that, very soon).

Tests are passing locally for me. Let's see what the testbot says.

The tests had 2 PHPLint fails. I fixed one, but the other was intentional, testing potx on files with syntax errors. So, I moved the syntax error to a separate file with a '.txt' extension, hopefully we can both keep the test, and prevent the phplint fail.

Added two helper methods to allow parsing and testing PHP content directly, and included the code with the syntax error as a string directly in the test.

The tests weren't picked up. The "TestConstraint.php" file, which is only meant to be parsed by potx, seems to have caused the testbot to throw an error.
Moved the "TestConstraint.php" and "LanguageManager.php" contents into the potx test class too, to hopefully prevent more errors on the testbot.

All the tests passed. Yay!
Adding an interdiff for all the changes this time.

I'll get back to this in a few days, to finish any remaining work, and if possible, commit this. Feel free to review the patch and all the changes here.

Yay, thanks for working on this! Some relatively minor notes:

1. +++ b/tests/modules/potx_test_8.module
   @@ -22,7 +15,7 @@ function potx_test_8_example() {
   -  $d = new TranslatableMarkup('TranslatableMarkup string with long array context', [], array('context' => 'With context']));
   +  $d = new TranslatableMarkup('TranslatableMarkup string with long array context', [], array('context' => 'With context'));
   

   Was this meant to test an error condition? drumm usually adds fail tests to ensure that the parser does not break on problems like this. Would be nice to dig up + add a comment here if so. Instead of "fixing" it.

2. +++ b/potx.inc
   similarity index 80%
   rename from tests/potx.test
   
   rename from tests/potx.test
   rename to src/Tests/PotxTestCaseTest.php
   

   What about just PotxTest?

3. +++ b/src/Tests/PotxTestCaseTest.php
   @@ -1,30 +1,28 @@
   +use Drupal\simpletest\WebTestBase;
   

   Are we doing any webtesting in this test or can it now be a unit test maybe?

No changes are made to the UI module itself. I assume that is not yet ported / not yet working on Drupal 8?

1. The line was added in 2598910#15. It doesn't mention being an intentional syntax error, and it's really similar to the existing syntax error case. Still, I can add it back if you like.

2. Done.

3. I tried converting them to a unit test, but PHPUnit marked all of the tests as "risky", due to the way potx modifies a lot of global variables during run. So, they might not be suitable as unit tests.

The UI was ported in the earlier patches, so it was missing from the latest interdiff, but you can review it in the full patch.

1. When generating an export, the page crashes:
   

   The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Core\Database\DatabaseExceptionWrapper</em>: SQLSTATE[42S02]: Base table or view not found: 1146 Table &#039;d8.languages&#039; doesn&#039;t exist: SELECT language, name, plurals, formula FROM {languages} WHERE language = :langcode; Array
   (
       [:langcode] =&gt; nl
   )
    in <em class="placeholder">_potx_get_header()</em> (line <em class="placeholder">639</em> of <em class="placeholder">modules/potx/potx.inc</em>).

2. When visiting /admin/modules the page generates a notice:
   Warning: include_once(includes/locale.inc): failed to open stream: No such file or directory in include_once() (line 563 of /[...]/modules/potx/potx.inc).

Attached patch implements the following changes:

1. Extracting translations for a specific language including existing translation strings is now functional
2. Resolved notices from loading non-existing files.
3. Updated a number of coding standards issues

A few disclaimers:

• The language plural formula is currently not available in Drupal 8. I have raised an issue to fix this: #2882617: String version of plural formula is not available, exported .po files contain an incorrect default. The language formula is currently hardcoded to Dutch pending the fix in Drupal core (sorry)
• Exporting translations depends on a number of functions that have been removed in Drupal 8, specifically _locale_export_string() and _locale_export_wrap(). Since potx.inc is to be changed as little as possible between the different branches, I have included these functions conditionally from potx.locale.inc

Thanx for this module - it is really needed ;)
According the plural formula:

• Since I'm trying to keep my core "un-patched" - would it be possible to just ignore the plural in extraction (maybe a warning?)
• Or even better: just ignore the translation and export singular + plural untranslated (maybe with a comment/warning?)
• And what do you mean by "The language formula is currently hardcoded to Dutch"?

I took a look at _potx_translation_export() in potx.inc - maybe you can point me in the right direction?

* Reroll with latest changes
* Add drush 9 command
* Remove drush 8 command
* Adjust weight for task
* Add is_array check in _potx_schema_reverse_lookup to prevent error in case $yaml is not an array
* Add missing _locale_export_remove_plural to D7 legacy functions
* Remove hook_reviews()
* Switch from moduleHandler->getModuleList() to system_rebuild_module_data(), as this retrieves also inactive modules
* Switch from fieldset to details in form

Patch and interdiff attached, please review.

Adding a composer.json with a drush extra section.

The last patch doesn't apply cleanly on 7.x-3.x and 7.x-3.0-alpha2. Also not so sure about removing the drush 8 command and having only a drush 9 command as people are yet experiencing problems with drush 9, depending on the core version.

I would like to use the command drush potx single --modules=mymodule,myothermodule --api=8

I get an error with the drush 9 command with #21.

[error]  Error: Call to undefined function Drupal\potx\Commands\drush_print_table() in Drupal\potx\Commands\PotxCommands->potx() (line 100 of /var/www/html/web/modules/contrib/potx/src/Commands/PotxCommands.php) #0 [internal function]: Drupal\potx\Commands\PotxCommands->potx('single', Array)

That said the patch should apply to the 8.x-1.x branch which it does and #18 did work with drush 8.

I think we should focus on a MVP for the patch, commit it and do incremental improvements in followup issues. The patch is already quite big and will not become easier to review. To support different drush versions or even drupal console we should put all the logic in a cli agnostic class and have the commands just simply wrap those methods. See the nuvole blog post about that for config split.

There is a wrong (or missing?) language in the extract page: /admin/config/regional/translate/extract

Steps to reproduce:
1-Install a monolingual French drupal site (8.5)
2-After having added some content, add a second language (English) as well as the necessary l10n modules and potx
3-In the extract page, under "Template language", I can see only 2 options:
-Language independent template
-Template file for French translations

As you can see "Template file for French translations" should be "Template file for English translations", because I have a french site that needs to be translated into English.

FYI, in the export and import tabs, I have the 2 languages...

I think that #24 is linked to: https://www.drupal.org/project/drupal/issues/2957548

...all the strings coming from the PHP files of my project (module/theme: t("a string")) and all the strings coming from my twig files {{ 'a string'|trans }} are seen by Drupal as English. Even if your site have been made in French as the first language

Thus my question is: could we tweak potx to make it think that the original strings (of the interface) are French strings (with the ability to export/translate/import)

Regarding #24 and #25, after some digging, it seems that the only thing that Drupal brings on the table for this situation is a checkbox/option in /admin/config/regional/language/edit/en that allow to translate MANUALLY in the UI the Original strings (French strings, but seen as English) into English. [this is not suitable for a large site]

Thus, if I understand correctly POTX doesn't handle this feature.
If it is correct, let me know if I should add a request feature for that problem.
I could be wrong, but to handle this option, POTX has to do the following

1- Extract all the French strings coming from the php/twig files into a PO file
2- After the translation of the PO, POTX will have to generate brain new php/twig file by having replaced the French strings by the corresponding English Strings
3- The webmaster will replace the "old" php/twig files by the newly created (with English string inside)
4- POTX will swap the language of the PO file created by step 1 (FR=>EN to EN=>FR)
5- POTX will import the swapped PO file into the Site to translate it into French

...What do you think?

@DuneBL afaik Potx (and Drupal) considers _all_ strings in _all_ files being english - as that is the convention in Drupal world.

@sanduhrs, yes, this is what I just discovered.
The fact that there is (AFAIK) no process to handle the following use case is not "non English speaker" friendly:
1-Create a monolingual non English drupal site (You will have to create source file in another language than English)
2-After a while add English as a second language for the site
=>You are stuck with a "there is no process for such a thing, except doing a lot of manual work..."

This is why I try to find a process with the help of POTX (or TMGMT?) because I think I am not alone in this case.

I understand why this rule (considers _all_ strings in _all_ files being English) exists... but, we need to find a good process when the files are not in English...

As a side note, I think that this rule could be adapted with the following idea:
TWIG: add a language tag at the top of the file
MODULE/THEME php: add a language entry in the info file

But, this would be a major change in Drupal Core that doesn't have to be discussed here (I think)

@DuneBL Actually I don't think this is in any way useful. We always start with english files and whether or not you do a non-english install the language in the files doesn't change. That's the reason we always start from english and the reason the convention exists.
Please discuss this in the support forums or create yourself a seperate issue.

This is the wrong place, thanks.

@sanduhrs I will add this request in the Drupal Core issue queue
But I don't agree that allowing POTX to handle this use case is not useful.
If you are German, and if you would like to create a German monolingual site, you will have German string in your files...
You could invest in a retro translation in English, but this is not friendly/easy and don't have any purpose.
This is why I think a process that handle this through POTX would be useful.

Any chance to move forward with something/anything here?

Like many others, we currently need to resort to https://github.com/kgaut/drupal-potx, which seems to be different from the patches here, and it's not clear what the leading implementation is.

It would be great if the most mature code (from here or kgaut or wherever) would be committed to 8.x-1.x to re-establish an official 8.x-1.x mainline that everyone can test and contribute to. (Alternatively, officially mark it as dead for D8 and link to kgaut instead.)

I'll take a look at this and other issues in the queue, in all of next week, and try to review/commit as much as I can.

@herom progress would be great, thanks alot!

I have reviewed and updated the patch at #2611298-21: Port potx to Drupal 8.
I brought back the Drush 8 command file, added a temp fix for the plural forms in .po headers until #2882617: String version of plural formula is not available, exported .po files contain an incorrect default gets in, and fixed the Drush 9 table output.
I also left some unrelated changes out, but they can be discussed in separate issues (for example, listing modules that are not installed in the extract form).

I'll take a look at kgaut's changes next week. Feel free to review the current patch. Thanks.

Removed the constant array, which doesn't exist in PHP 5.5.

Fixed syntax errors.

Reverting some changes. Also uploaded another interdiff from #21.

Here's a big update, but most of it is to pass the coding standards checks.
There is still some kind of issue with nightwatchjs testing (which isn't used here) that causes the tests to fail on Drupal 8.6. If anyone has an idea why it fails, it could be really helpful.

Tests are passing on Drupal 8.5, so I'm moving the patch to "needs review".
I'll also start looking at kguat's code.

Any idea how to review 1M patch file? I think it will be better to provide a link to github repo where fresh code lives

There you go: https://github.com/sanduhrs/drupal-potx

I tried running the following command with
1. Drupal 7, Drush 8
2. Drupal 8, Drush 9
drush potx --api=7.x --folder=drupal-7.59 > log.txt

it produces the exact same general.pot files.
And only slightly differing log files.

Files 447
Strings 2562

Attached are the missing two files for the previous post.

Same thing for the D8 codebase
1. Drupal 7, Drush 8
2. Drupal 8, Drush 9
drush potx --api=8.x --folder=drupal-8.5.3 > log.txt

The Drupal 7, Drush 8 version produces a different version than
the Drupal 8, Drush 9 version:

Drupal 7, Drush 8
Files 5909
Strings 8633

Drupal 8, Drush 9
Files 5909
Strings 8754

Apparently the version with our patch finds 121 more strings than the legacy version of potx.
Not sure if this is an issue or a feature :)

Hiding the files from the summary, to highlight the patch.

RE #49: Definitely a feature :) The extra strings are from optional config. But when I tested the same combination locally, it could find those strings, and there are tests for optional config in Potx 7.x. So it would be great if anyone else could test the same combination (run potx 7.x-3.x with drush 8, on a drupal 8 directory), and report the number of strings found by potx.

RE #45: A big part of the patch (~600KB) is just removing the vendor directory. So, I'm uploading another patch without the vendor directory removal. I have also split the coding standards fixes to a separate diff. The smaller patch should be easier for review.

I have also looked at kgaut/drupal-potx repository, but I'm going to apply those changes in a separate issue after this one.

I'm going to commit the patch in 3-4 days, so reviews are welcome until then. Thanks.

I have reviewed the code. I have comments, but I prefer to get the code in to get a basic working D8 version and improve in follow-ups. I don't want to block the issue now, it is pending for long.

Some points for follow-up:

• Long methods; may be broken down for better separation and maintainability
• Some commented-out debug() statements
• Use of private/helper (_potx_*()) procedural functions as public functions; rename these function()
• New procedural functions are being introduced (potx.locale.inc); Can this be convert to service

OK then. Committing this now. I decided to break this into three commits, similar to the patches in #52 to keep the history a bit cleaner.

Thanks to everyone's help and patience on this issue, and sorry that it took me so long to get it committed.

I'm going to start looking at the current issue queue, apply some of the changes in kgaut's repository, and also open up a few followup issues.