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 migrate & migrate_drupal modules

File Bounds:

core/modules/migrate/*
core/modules/migrate_drupal/*

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 migrate & migrate_drupal modules.

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
#3 replace_placeholder-2559467-3.patch8.13 KBjoelpittet

Comments

justAChris created an issue. See original summary.

justachris’s picture

justachris commented
Issue summary: View changes
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new replace_placeholder-2559467-3.patch8.13 KB
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community

This patch changes both migrate and migrate_drupal modules.

There are quite a few !placeholders in Drupal\migrate_drupal\Tests\ namespace, but those files seem to be generated to support migration testing and don't need to be checked for safety by Drupal 8.

Looked for other !placeholders in strings using NetBeans and my eyes. (!? as a wildcard expression)

Couldn't find any.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: Replace !placeholder with @placeholder in migrate module » Replace !placeholder with @placeholder in migrate and migrate_drupal modules
catch’s picture

catch
he/him
Primary language English
commented

Shouldn't we also do the tests here? Those strings don't need to be checked for safeness, but the idea is to remove usages of !placeholder entirely eventually.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

I'm not sure how the migrate tests work. I assume they're using strings generated from D6 and D7, and they might just be testing whether a conversion away from !placeholders happens or not during migration.

justachris’s picture

justachris commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing
Related issues: +#2559459: Replace !placeholder with @placeholder in User module

My mistake on #4, migrate_drupal should have been included in the scope of this issue initially. Updated IS to clarify. This includes the tests found in those directories.
On comments #6,7 we should be careful about how !placeholder is used. In some areas of the migrate module (and other modules) they indicate using a token, which I think is out of the scope of these updates. This was also noticed in #2559459-7: Replace !placeholder with @placeholder in User module.

Additionally, can we get manual testing here to make sure that the changes don't affect escaping on the output?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

May be able to test some of this easier with https://www.drupal.org/project/migrate_upgrade

@mikeryan1 pointed this out to me on IRC #drupal-migrate

justachris’s picture

justachris commented
Status: Needs review » Postponed

Postponed on determining plan in parent #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand and then analyzing whether this issue still makes sense.

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.

sutharsan’s picture

sutharsan commented

Patch now merged into #2506445-140: 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 »