Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#47161 closed defect (bug) (fixed)

Do not break strings to avoid HTML tags in them

Reported by: dimadin's profile dimadin Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.2.1 Priority: normal
Severity: normal Version:
Component: I18N Keywords: has-patch i18n-change
Focuses: Cc:

Description

[44986] introduced sentence that was broken in two strings to accommodate link tag. This was corrected in [45099], however it was reverted in [45170].

From what I see, this link should not have been here in the first place. In other tests, links are under actions key of the result array, not under description, and whole action sentence is link. This would remove need for placeholders in a string.

In attached patch, I moved link to actions key and also simplified text of link (other external links here have no description about linked page).

Attachments (3)

47161.diff (2.0 KB) - added by dimadin 5 years ago.
47161.2.diff (1.5 KB) - added by ocean90 5 years ago.
47161.3.diff (1.5 KB) - added by ocean90 5 years ago.

Download all attachments as: .zip

Change History (15)

@dimadin
5 years ago

#1 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.2.1
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#2 follow-up: @Clorith
5 years ago

The link location is actually intentional here. The link is included for context in the description on where the requirements and recommendations come from, and belongs as part of the description.

The action section is for actionable items to resolve a failing test, which this link is not.

#3 in reply to: ↑ 2 @SergeyBiryukov
5 years ago

Replying to Clorith:

The link location is actually intentional here. The link is included for context in the description on where the requirements and recommendations come from, and belongs as part of the description.

The action section is for actionable items to resolve a failing test, which this link is not.

My initial thought was that this link isn't much different from "Learn more about updating PHP" and "Read more about why you should use HTTPS", which are both placed in the action section.

On second thought, I see the difference: the latter two are user-oriented and located in HelpHub, while the link in question is a more technical-oriented list of recommended PHP extensions in the Hosting team handbook.

If we had a HelpHub article on managing PHP extensions, that would be a better fit for the action section. For now, let's just fix the i18n best practices regression in [45170].

#4 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 45331:

Site Health: In PHP modules test, ensure the description is translated as a whole sentence, not as separate string parts.

Props dimadin, SergeyBiryukov.
Fixes #47161.

#5 @SergeyBiryukov
5 years ago

In 45332:

Site Health: In PHP modules test, ensure the description is translated as a whole sentence, not as separate string parts.

Props dimadin, SergeyBiryukov.
Merges [45331] to the 5.2 branch.
Fixes #47161.

@ocean90
5 years ago

#6 follow-up: @ocean90
5 years ago

  • Keywords i18n-change added
  • Resolution fixed deleted
  • Status changed from closed to reopened

I think we should move the leading space for the screen reader text into the argument, see 47161.2.diff.

#7 @ocean90
5 years ago

#47300 was marked as a duplicate.

#8 @tobifjellner
5 years ago

The change in comment 6 is almost correct. But it still will add a space between "the team handbook" and the following stop.
You need to put the space inside the span "screen-reader-text", i.e. just before the %s.

Last edited 5 years ago by tobifjellner (previous) (diff)

@ocean90
5 years ago

#9 in reply to: ↑ 6 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to ocean90:

I think we should move the leading space for the screen reader text into the argument, see 47161.2.diff.

I've considered this, but [45331] didn't add any new spaces that weren't already there in [45170], so it's not a regression.

Looking at other strings with the dashicons-external icon, they all have the space before the screen reader text and the icon:

  • Learn more about updating PHP
  • Read more about what WordPress requires to run.
  • Get help resolving this issue.
  • Read about debugging in WordPress.
  • Read more about why you should use HTTPS

Same for the strings added earlier in wp_dashboard_events_news(), wp_dashboard_php_nag(), and wp_direct_php_update_button():

  • Meetups
  • WordCamps
  • News
  • Learn more about updating PHP
  • Update PHP

I've opened #47303 to handle them in a consistent manner, since that's out of scope for this ticket.

#10 @ocean90
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It's not about the position of the space, it's whether the space should be part of the translatable string or not. It should not. That's separate from #47303.

#11 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 45346:

Site Health: In PHP modules test description, move the space before the screen reader text out from the translatable string.

Props ocean90, tobifjellner.
Fixes #47161.

#12 @SergeyBiryukov
5 years ago

In 45347:

Site Health: In PHP modules test description, move the space before the screen reader text out from the translatable string.

Props ocean90, tobifjellner.
Merges [45346] to the 5.2 branch.
Fixes #47161.

Note: See TracTickets for help on using tickets.