Documentation problem with filter_example.module

P.P.S. While you are at it, I suggest putting the comment block headed

 /*
  * Foo filter

after the implementation of _filter_example_information().

Sorry to do this piecemeal, but I have more problems with the comment block

/*
 * Foo filter
 *
 * Drupal has several content formats (they are not filters), and in our example
 * the foo replacement can be configured for each one of them, allowing an html
 * or php replacement, so the module includes a settings callback, with options
 * to configure that replacements. Also, a Tips callback will help showing the
 * current replacement for the content type being edited.
 */

1. I think the term "content formats" should be replaced by "text formats."
2. What does "html or php replacement" mean? I tried using <?php 7+5 ?> and variants as the replacement text, and got odd results: the first "foo" was replaced with the default "bar", and the rest of the text was deleted.

The function _filter_example_filter_foo_tips() contains the line

return t('<em>foo</em> replaced with %replacement.', array('%replacement' => $replacement));

I would use

return t('%foo replaced with %replacement.', array('%foo' => 'foo', '%replacement' => $replacement));

since 'foo' should not be translated. Ditto for the long form of the tip.

There is a typo in the comment block

/*
 * Time filter process callback
 *
 * Now, in the "process" step, we'll search for our escaped time tags and
 * to the real filtering: replace the xml tag with the date.
 */

The word "to" on the last line should be "do".

Yes, I have made patches before, and I am happy to make one for this issue. I just need a hint for how to get this file. AFAIK it is not part of the distribution tarball. Also, do you like my use of htmlspecialchars() or should I just use

      '!tag' => '<time />',

in the patch?

The attached patch fixes the problems discussed in this issue:

1. HTML tags are properly escaped in _filter_example_information().
2. The comment block headed * Foo filter is moved after the implementation of _filter_example_information().
3. In that comment block, "content formats" is replaced by "text formats" and the phrase "allowing an html or php replacement" is removed. Line breaks are adjusted.
4. In _filter_example_filter_foo_tips(), the string 'foo' should not be translated, so it is replaced with a token. Since the same two tokens are used in the long and short versions of the tip, a new variable is used as the second argument of t().
5. In the comment block headed * Time filter process callback, the phrase, "to the real filtering" is changed to "do the real filtering".

In (1) I did not use htmlspecialchars() as I originally suggested. (KISS)

P.S. Thanks to jhodgdon for moving this to the right queue and to rfay for the git instructions.

I noticed a typo in one of the comments, so the attached patch includes one more chunk.

I agree that 'foo' is not configurable. The reason I tokenize it is to protect it from accidental translation. I admit it is unlikely that a French translator would turn it into 'Chaque instance de "feu" dans le texte d'entrée ...' (replacing "foo" with something that sounds similar) but I think it is possible. Maybe a more realistic concern is the Russian translation: Google suggests 'Каждый экземпляр "Foo" в ввода текста', and capitalizing "foo" breaks it.

I count the contents of _filter_example_filter_foo_tips() (or any other help text) as documentation. I still do not think that my patch made 'foo' "replaceable."

But you are the boss, so I am submitting a patch with the changes to _filter_example_filter_foo_tips() reverted. In other words, the same patch as in #10, but without item 4 from comment #7.

Have to change the status to get the patch tested. Still learning ...

droplet, thanks for the review.

I agree that using @tag makes it easier to read. The change you suggest to _filter_example_filter_time_tips() will make it easier to read, too, but I would not say it "needs fix" since it generates the same HTML either way.

I will make both the changes you suggest. What do you think of replacing "foo" with a token (or placeholder)? See comments 2, 11, 12 above.

The attached patches implement the two suggestions in #16. They are based on the patch in #10, but the second patch (with "minimal" in the file name) passes 'foo' as part of the first parameter to t() instead of using array('@foo' => 'foo'), as requested in #11.

I still think that it makes sense to pass 'foo' in the second parameter of t() in order to prevent accidental translation. This does not make the string 'foo' "configurable": in fact, it stops the translator from accidentally "configuring" it in the filter tips and elsewhere.

Am I missing some important difference between @foo and %foo? Comment #16 suggests a difference, but my understanding is that the % form just adds <em> tags.

I think it is time to edit the summary ...