Problem/Motivation

See #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only

search_excerpt

Proposed resolution

search_excerpt() contains an instance of a !placeholder, but there is no corresponding argument passed for !placeholder to be replaced with. The string is only used as a way for search to try to get information on how a translator would translate an ellipsis. Therefore, whether it uses !placeholder or @placeholder makes no difference.

So, it can be simply changed from one to the other.

Remaining tasks

A followup is needed because the current code in search_excerpt() is incompatible with some translation techniques, including adding markup to the string, and it has hardcoded assumptions about what a string of excerpts would look like in a given language.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#3 2568611.3.patch743 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

alexpott’s picture

This one is interesting. It's not using !placeholder as a placeholder :)...

  // Combine the text chunks with "…" separators. The "…" needs to be
  // translated. Let translators have the … separator text as one chunk.
  $ellipses = explode('!excerpt', t('… !excerpt … !excerpt …'));

It is using it as a way to allow translators to translate the multiple .

alexpott’s picture

Status: Active » Needs review
FileSize
743 bytes

I guess though there is no harm in replacing ! with @

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Haha, not at all, but yeah it makes sense I could imagine something like "..." depends on the language, or at least could be useful to be translatable.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Interesting one.

Committed/pushed to 8.0.x, thanks!

  • catch committed 5e63a99 on 8.0.x
    Issue #2568611 by alexpott: Replace remaining !placeholder for Non-URL...

Status: Fixed » Needs work

The last submitted patch, 3: 2568611.3.patch, failed testing.

alexpott’s picture

Status: Needs work » Fixed

pifr is slow.

dawehner’s picture

Seriously testbot

xjm’s picture

Issue summary: View changes

So I was really concerned that this change went in without testing, until @alexpott pointed out that there is no variable passed to placeholders at all, and SafeMarkup will never even be used because there are no placeholder arguments.

However, the reason I asked for this issue to be split out in the first place is that there is serious code smell in this function related to its use of t() and otherwise. I was hoping it would be discussed in this issue, but I think we at least need an inline comment with that t() documenting why it's used and ideally linking the followup to fix the whole function. @catch located #2212323: Make a way to better control search result display, but there is also a specific bug/problem WRT the translatability.

jhodgdon’s picture

I don't understand what the translation problem is. Can you please file an issue explaining what the "code smell" you detected is?

The thing is, the search_excerpt() function needs a way to output

... Some text from a node ... some more text ... some more text ... some more text ...

and to allow other languages to perhaps use a more appropriate character than ... to separate the excerpts.

So that is why we do t(), as documented in the comment directly above the line changed in this patch:

   // Combine the text chunks with "…" separators. The "…" needs to be
   // translated. Let translators have the … separator text as one chunk.
-  $ellipses = explode('!excerpt', t('… !excerpt … !excerpt …'));
+  $ellipses = explode('@excerpt', t('… @excerpt … @excerpt …'));

I do not know of any other way to let translators provide the appropriate character(s) except by calling t()... do you?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.