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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Status: Active » Needs review
FileSize
14.6 KB
jhodgdon’s picture

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

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

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

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

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

Status: Needs work » Needs review
FileSize
743 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

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

Status: Needs review » Needs work

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

justAChris’s picture

Adding related issue, likely cause on fail

jhodgdon’s picture

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

justAChris’s picture

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’s picture

Status: Needs review » Closed (duplicate)
xjm’s picture