Integrate wporg custom warnings

[amieiro]

The GlotPress installed at WordPress.org has a plugin that implements some custom translation warnings. It adds some new warnings and makes changes to some core Glotpress warnings.

This PR migrates those warnings from the custom plugin to GlotPress.

The warnings are managed in GlotPress in two classes, both in the "gp-includes/warnings.php" file:

• GP_Translation_Warnings, used to handle translation warnings.
• GP_Builtin_Translation_Warnings, used to register built-in translation warnings. The common warnings are defined in the methods of this class.

This PR:

1. Adds a new CLI command (GP_CLI_Wporg class, available at "gp-includes/cli/wporg.php") to be able to set the installation as the wporg installation. This CLI command sets or removes an option variable (gp_is_wporg) in the options table. The usage of this command is very simple:

   • To set the installation as the wporg installation, execute wp glotpress wporg set
   • To remove the installation as the wporg installation, execute wp glotpress wporg unset

2. Adds or updates the methods from the wporg plugin that can fit in the generic solution in the GP_Builtin_Translation_Warnings class:

   • Adds the warning_mismatching_urls() method. This method adds a warning for changing plain-text URLs. This method allows:

     • Change the scheme (https <=> http).
     • Include/remove a trailing slash in the URL.
     • For some domains to change to a subdomain.

     This method has a hook to update the list of domains with allowed changes (empty by default). More info in the point 3.

   • Adds the warning_unexpected_sprintf_token() method. This method adds a warning for adding unexpected percent signs in a sprintf-like string. This is to catch translations for originals like this:

     • Original: <a href="%s">100 percent</a>
     • Submitted translation: <a href="%s">100%</a>
     • Proper translation: <a href="%s">100%%</a>

   • Updates the warning_tags() method with the wporg check rules:

     • URLs (href + src) are run through the previous method (warning_mismatching_urls()).
       • The domain may change for some safe domains.
       • The protocol may change between https & http.
       • The URL may include/remove a trailing slash.
     • The value of translatable/url attributes is excluded from the error message if it's not related to the issue at hand.
     • Tags are sorted, <em>One</em> <strong>Two</strong> can be translated as <strong>foo</strong> <em>bar</em> without generating warning.
     • East Asian languages can remove emphasis/italic tags.

   • Adds two new rules to the warning_tags() method, not present in the wporg plugin:

     • 1st rule checks if the HTML tags are in correct order. This one warns about HTML tags translations in incorrect order. For example:
       • Original: <a></a>
       • Translation: </a><a>
     • 2nd rule checks whether links that are not URL or placeholders are equal or not. For example:
       • Original: <a href="%s" title="Blimp!">Baba</a>
       • Translation: <a href="javascript:%s" title="Блимп!">Баба</a>

3. Adds a new class (GP_Wporg_Translation_Warnings, available at "gp-includes/wporg/warnings.php") to handle wporg installation specific elements that do not fit in the generic plugin. Currently, this class has two relevant methods:

   • add_wporg_allowed_domains() adds the allowed domains in the GP_Builtin_Translation_Warnings class using the gp_allowed_domain_changes hook present in the warning_mismatching_urls() method. The current domains with allowed changes to own subdomains are:
     • wordpress.org
     • wordpress.com
     • en.gravatar.com
     • en.wikipedia.org
   • gp_add_all_warnings() method adds all warning methods from this class in the GP_Builtin_Translation_Warnings class using the gp_add_all_warnings hook in the add_all() method.

   Currently, this new class has only a new warning method, warning_wporg_mismatching_placeholders(), that adds a warning for changing placeholders. This method only supports placeholders in the format of '###[A-Z_]+###'. This warning method is so specific that only fits in the wporg installation.
   The two new hooks present in the GP_Builtin_Translation_Warnings class (gp_allowed_domain_changes and gp_add_all_warnings) are only "hooked" if a variable (gp_is_wporg) is set in the options table, setting the installation as the wporg installation.

   Note: This PR includes the changes of this other PR.

Architecture

This architecture, which separates the common warnings from the wporg warnings and injects them via hooks, is extensible, because:

• If the plugin needs a new warning for all the installations, it can be placed in the GP_Builtin_Translation_Warnings class and its tests in the GP_Test_Builtin_Translation_Warnings class.
• If the warning only fits for the wporg installation, it can be placed in the GP_Wporg_Translation_Warnings (and its tests in the GP_Test_Wporg_Warnings class) and inject the code using a current hook (or a new one if it would be required) in the GP_Builtin_Translation_Warnings class.
• This approach allows to eliminate the need for a custom plugin, because all its business logic is injected using hooks, and it is easier to customize with new warnings.
• This approach allows to keep all the business logic of the wporg installation in its own classes and folders, isolated from the main plugin. This allows not to run the custom code for the wporg installation on a normal installation, which will not have the option table variable (gp_is_wporg) enabled by default.

Tests

I have added new tests for these new or updated methods. I have divided the tests in two files:

• GP_Test_Builtin_Translation_Warnings class (tests/phpunit/testcases/test_builtin_warnings.php file). This is an existent class. I have created or updated the test methods to adapt it to the new logic. These tests are for the common functionality.
• GP_Test_Wporg_Warnings class (tests/phpunit/testcases/test_wporg/test_wporg_warnings.php file). This is a new class to test the new methods that will only be executed in the wporg installation.

Test improvements

• This PR adds a new method (assertHasWarningsAndContainsOutput()) in both test classes that replaces and extends the functionality of the assertHasWarnings() method. This method is responsible for checking the same as the assertHasWarnings() method and also checks the text returned by each warning, so we can assure the warning output text.
• This PR adds a new method (test_chained_warnings() in the GP_Test_Builtin_Translation_Warnings class) that checks that several chained warnings show the corresponding errors. This is a kind of integration test, where the different warning methods are tested working together.

[ocean90]

Thanks for working on this! What was your motivation to adopt the custom warnings?

Before going into a deep review, everything related to GP_Wporg_Translation_Warnings and the CLI command should be removed again as this doesn't belong to the plugin. There's nothing wrong with still using a plugin on wordpress.org which can customize the warnings.

[ocean90]

Looking at the two specific warnings, I think they can be integrated too without the need to set a wporg flag. The domains should only be extendable which they are thanks to the filter and we'd a good name for warning_wporg_mismatching_placeholders, maybe warning_hashtag_placeholders or warning_named_placeholders?

[amieiro]

I have removed all the wporg parametrization, including the CLI command, and I have integrated the domains and the specific warning in the builtin warnings. I have named the moved method as "warning_mismatching_placeholders", to maintain the same name that it has now in the wporg plugin.

[ocean90]

I have named the moved method as "warning_mismatching_placeholders", to maintain the same name that it has now in the wporg plugin.

I still don't think that this is a good name when you compare it with warning_placeholders. Just from reading the name I'd expect that both are about the same placeholders. But one is about PHP placeholders and the new one about "named" placeholders. Maybe we should consider renaming warning_placeholders to warning_php_placeholders but that's something for a separate issue. With warning_mismatching_placeholders it would also get complicated to find another name if we plan to support another type in the future. What do you think?

[amieiro]

I think we can change warning_mismatching_placeholders to warning_translation_placeholders so that whoever sees it understands that this method checks translation placeholders. In another PR we could change warning_placeholders to warning_php_placeholders. Do I make the change?