Translatable strings review

Can you give an example of what could go wrong?

I must be misunderstanding something. My purpose in writing the string as it is was to keep HTML out of it. Your suggested change puts the HTML back in the string. A link to the discussion would be helpful.

I'm going to save this for the Drupal 6 branch.

Sorry for the "won't fix." I'm changing this to by design for the Drupal 5 version. Hopefully the guidelines for converting modules to Drupal 6 will address this question.

I am changing the status to fixed to make the report close automatically after 2 weeks.
I am changing the category as the report doesn't describe a bug of the code.

In this case, this is a duplicate of #345654: Correct the calls to t().
Anyway, the report topic was about Translation may be context sensitive, and not about the incorrect usage of t().

The report refers to 5.x-1.2 which is an obsolete version.
The only versions which take fixes are 5.x-2.x-dev, and 6.x-1.x-dev which are changed to reflect the code I am using on the version of the module I am developing internally.

I am changing the category because the report is about the choice of strings to use; the change proposed was to make easier for who translate the module to a different language, not because the module was not working correctly.
As a matter of fact, the versions which receive the bug fixes are available only in English.

The incorrect use of t() is another issue, that is completely separated from the current one. There is already a report about that.

There we go.
The code has been changed in 5.x-2.x-dev; therefore the patch does not apply anymore.

Repeating myself, the use of a word in a UI is not a bug. If it would be so, then all the times a module uses a word that for me is not correct, I could report a bug.

The patch also changes Site map into XML site map which was object of a different report to which Darren Oh replied that he was not going to change it if he would not have seen a real need to change it.

The line return 'unsupported type '. $type .'for xmlsitemap_placeholder'; doesn't need to be localized because it's not shown to the end user. That function, that is a port of a function existing for Drupal 6 (db_placeholder()) should return a placeholder to be used for the data type passed to the function; when the function get an invalid argument, it returns that value that will cause the query to fail, and will be only visible in the log that reports the query which caused the error.
Being reported inside a query that use English words, it doesn't have any meaning to be translated; if that would be the case, then also the SQL queries that update.php shows in the final report would need to be translated.

For the change from Site map to XML site map I will open a task report; if that task is reported by other people to be necessary, then I will make the necessary changes.

No, it is not.
The string returned should be normally something like '%s', %f, used to build the query. When the function gets a value for $type that the function is not able to handle, it returns a value that makes the query fail, as it takes the assumption it gets a wrong value because something else is wrong in the code, somewhere.

Anyway, the Drupal 6 code doesn't return a translated string in the function I back ported to Drupal 5, so I don't see any reason to do it in the code I wrote, which is merely a copy of code existing in Drupal 6.

Well, I could close it because the only string subject of the patch was already modified.
Patch (code needs work) would mean there are some changes to be applied; in this case, the change would have made the patch an empty file.

This report is partially matching #345654: Correct the calls to t().
You keep to say they are two different issues, then in your patch there would not be any corrections that change the call to l() into a call to url() As that change is already reported in that task report, it should not corrected in this issue.

The patch needs to be changed as more code has been ported from the Drupal 6 branch I am developing internally.
The back-port also includes the use of translatable strings used in the priority settings that before used values like '1.0', '0.9', '0.8' without to pass them to t().

Having corrected the use of a wrong function, the category can now be changed; bug report usually refers to code bugs.

The description strings used in the implementation of hook_schema() are all passed to t() like is evident in the documentation; if you then see how the core modules implement that hook, you will see they all do the same (just to make an example, book_schema(), node_schema()).

This issue is referred to 5.x-2.x-dev; let us change the code in that version, and pass to 6.x-1.x-dev successively.

Your affirmation is not completely true, as the issue has not been fixed.

I would remind you that you were suggesting to to make translatable a string used by a port made by me, when even the original code passed that string to t().
Then I would make you notice that wrong patches are not accepted, whomever submit them; it could be even the Pope, but a wrong patch still is a wrong patch.

To complete, the report is for 5.x-2.x-dev which doesn't even have a schema API.

'@' should be used only for input got from external sources, and I am not sure that is always true when you use it.

I would suggest you to not submit any patches, as I am still porting the code I internally developed to the development snapshots.

Until the referring version is 5.x-2.x-dev all the patches must focus on that version; adding a patch for a version for which the issue has not been opened creates just confusion on who follows the report.

I hope we agree on this.

As far as I can read, the differences between the placeholders for t() are the following:

• !variable indicates that the text should be inserted as-is.
• @variable indicates that the text should be run through check_plain(), to escape HTML characters.
• %variable indicates that the string should be HTML escaped and highlighted with theme_placeholder().

That means that, if I don't need to pass the value which replaces the placeholder through drupal_validate_utf8(), or htmlspecialchars() (which is what check_plain() does) I would not use '@'.
It could even happen with the input received from the user, if the input is, i.e., converted into an integer with (integer) $input, and then it is used in a string (which normally happens if that value is output into a Drupal page). In such case both the functions (drupal_validate_utf8(), and htmlspecialchars()) are useless: a string which contains digit characters is surely valid UTF-8, and it surely doesn't contain HTML characters that must be escaped.

Therefore the assumption that '@' placeholder strings must be used by default must be reviewed.

This has been fixed in CVS for 5.x-2.x-dev.
I will change the code for Drupal 6 branch as well, later.

The code still uses the !variable placeholder when the value which replaces it is a constant string or it's a value that is an integer (I also added a simple check on the value returned by a function to be sure the value is an integer).

There is still something that needs to made better. The string '!sitemap submitted to !engine' (with '!sitemap' being replaced by 'Site map index', or 'Site map') should be changed, as it maybe opens again the problem of a translatable string given without a context; I don't know if this is the case, but it could be that site map is translated in more than one way in some languages.

The only solution I can find is to simply use Site map submitted to !engine. It could be confusing to read the same message repeated more than once in row in the log, but I don't think it's requested this level of detail, in this case.

I also applied the change I was talking before in CVS.
I will make the necessary changes for the Drupal 6 branch later; the commit for Drupal 6 will be delayed, as I am changing the development snapshot code to the code I was testing internally.

I am setting the status to fixed until I don't change the referring version to 6.x-1.x-dev.

The changes have been committed in CVS.