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 comment module

File Bounds:

core/modules/comment/*
Excluding hook_help()

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 comment module.

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

Comments

justAChris created an issue. See original summary.

justachris’s picture

Issue summary: View changes
joelpittet’s picture

Status: Active » Needs review
StatusFileSize
new10.68 KB
justachris’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing

We missed a couple of replacements:

CommentFieldAccessTest::testAccessToAdministrativeFields()
core/modules/comment/src/Tests/CommentFieldAccessTest.php:
Line 263:
// Check create-only fields.
    foreach ($this->createOnlyFields as $field) {
      // Check view operation.
      foreach ($permutations as $set) {
        $may_view = $set['comment']->{$field}->access('view', $set['user']);
        $may_update = $set['comment']->{$field}->access('edit', $set['user']);
        $this->assertEqual($may_view, $field != 'hostname' && ($set['user']->hasPermission('administer comments') ||
            ($set['comment']->isPublished() && $set['user']->hasPermission('access comments'))), SafeMarkup::format('User @user !state view field !field on comment @comment', [
          '@user' => $set['user']->getUsername(),
          '!state' => $may_view ? 'can' : 'cannot',
          '@comment' => $set['comment']->getSubject(),
          '!field' => $field,
        ]));
        $this->assertEqual($may_update, $set['user']->hasPermission('post comments') && $set['comment']->isNew(), SafeMarkup::format('User @user !state update field !field on comment @comment', [
          '@user' => $set['user']->getUsername(),
          '!state' => $may_update ? 'can' : 'cannot',
          '@comment' => $set['comment']->getSubject(),
          '!field' => $field,
        ]));
      }
    }
izus’s picture

Status: Needs work » Needs review
StatusFileSize
new7.88 KB

hi,
i deleted the part of hook_help as there is one mega patch to fix is in #2560783: Replace !placeholder with :placeholder for URLs in hook_help() implementations
i fixed the missing parts mentioned in #4
Thanks

justachris’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
StatusFileSize
new53.93 KB

Well that simplifies manual testing a bit. The only location that is not a test is in the meta information on a comment display. Since this specific text is visually hidden, including only a grab of the source, which matches exactly before the patch:
In Reply to comment meta

Comment #4 has been addressed and I don't see any other occurrences of !placeholder; all changes are in the scope of this module.
Updating IS to indicate separation of hook_help() from this issue. Good to go.

justachris’s picture

Issue summary: View changes
StatusFileSize
new100.18 KB

Wow, that last image was too scaled to be legible. Trying again:

Still RTBC

catch’s picture

Status: Reviewed & tested by the community » Postponed
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.

sutharsan’s picture

Status: Closed (duplicate) » Needs review
StatusFileSize
new978 bytes

Rerolling patch for easy migration into single patch at #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests.
Changing status for test bot. Do revert status after test.

sutharsan’s picture

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