Follow-up to #2559445: Replace !placeholder with @placeholder in aggregator module

Problem/Motivation

In order to make #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests approachable, we need to break it up into smaller chunks. This issue address !placeholder in the Search module

See #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand for complete motivation on removal of !placeholder

Proposed resolution

Replace !placeholder with @placeholder in the Search module.

core/modules/search/*

Remaining tasks

  1. Replace !placeholder with @placeholder. Refer to patch in #2506445-85: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests as that patch should have related update
  2. Ensure tests come back clean
  3. Manually test the update and post screen shot after patch, review source for any difference in escaping.

User interface changes

CommentFileSizeAuthor
#8 replace_placeholder-search-2559455-8.patch743 bytesizus
#2 replace_placeholder-2559455-2.patch14.6 KBjoelpittet

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new replace_placeholder-2559455-2.patch14.6 KB
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

This patch only affects:
a) Search module main help page
b) Help at top of the Search config page
c) Search results page (excerpts section)

So I tested it at simplytest.me and all of these are fine (all of the links work in the help, too). Good go go!

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I think this approach may be better instead of lots of patches with the same stuff in them.
#2560783: Replace !placeholder with :placeholder for URLs in hook_help() implementations

+++ b/core/modules/search/search.module
@@ -764,7 +764,7 @@ function search_excerpt($keys, $text, $langcode = NULL) {
-  $ellipses = explode('!excerpt', t('… !excerpt … !excerpt …'));
+  $ellipses = explode('@excerpt', t('… @excerpt … @excerpt …'));

This may be the only line needing commit here if the other one gets in first.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Let's make sure this doesn't get committed and break the other patch, since the other issue is being actively discussed and/or committed.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Actually I think we should definitely just deal with the non-hook-help parts in this issue, since even the question of whether to do it or not is being discussed on the other issue.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Fair, we may just close this or repurpose it or something and move that back to the "remaining" child issue because we split them based on size and this one is not meeting that criteria anymore.

izus’s picture

izus commented
Status: Needs work » Needs review
StatusFileSize
new replace_placeholder-search-2559455-8.patch743 bytes

Here is the patch with no hook_help parts as they will be addressed in #2560783: Replace !placeholder with :placeholder for URLs in hook_help() implementations
Thanks

Status: Needs review » Needs work

The last submitted patch, 8: replace_placeholder-search-2559455-8.patch, failed testing.

izus’s picture

izus commented

test fails due to file permissions in file migrate component.
that have nothing to do with current patch i think

Status: Needs work » Needs review

izus queued 8: replace_placeholder-search-2559455-8.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: replace_placeholder-search-2559455-8.patch, failed testing.

justachris’s picture

justachris commented
Related issues: +#2562453: EntityFileTest fails randomly

Adding related issue, likely cause on fail

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Postponed

Yeah but let's not call it Related. It's causing all kinds of tests to fail the same way.

Also please read #7. Once the hook_help() issue is fixed, this one will probably be closed so making another patch or testing it is premature.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Related issues: -#2562453: EntityFileTest fails randomly
justachris’s picture

justachris commented
Status: Postponed » Closed (duplicate)

Closing this, splitting by module was not the ideal approach to removing !placeholder. Marking as duplicate of #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand, since the chosen approach is / will be outlined there, please refer to it for any additional action.

Status: Closed (duplicate) » Needs review

Sutharsan queued 8: replace_placeholder-search-2559455-8.patch for re-testing.

sutharsan’s picture

sutharsan commented
Status: Needs review » Closed (duplicate)

Status back to 'Closed (duplicate)'. Patch now included in #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests.

xjm’s picture

xjm
she/her
Primary language English
commented
Parent issue: #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand »