WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 12 days ago

#46946 closed enhancement (fixed)

Site Health: Text adjustment (add a link and remove "we")

Reported by: birgire Owned by: leogermani
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: site-health has-patch
Focuses: Cc:

Description

It would be helpful, I think, to have a link to the Site Health Status page within this text:

"If we see anything here that could be improved, we will let you know on the Site Health Status page."

on the /wp-admin/site-health.php?tab=debug page.

Additionally it could be adjusted further to avoid the "We" self-reference, as suggested in #46057.

Also "anything here" is too general, in my mind.

cheers

Attachments (5)

46946.diff (993 bytes) - added by mukesh27 4 months ago.
Updated text.
46946-2.diff (1.0 KB) - added by subrataemfluence 4 months ago.
Added suggested link against text "Site Health Status page".
46946-3.diff (1.0 KB) - added by subrataemfluence 4 months ago.
Updated patch. I also excluded the word "page" from the link as per the original ticket description.
46946-4.diff (1.2 KB) - added by leogermani 2 months ago.
46946-5.diff (1.1 KB) - added by leogermani 2 months ago.

Download all attachments as: .zip

Change History (29)

#1 @Clorith
4 months ago

  • Milestone changed from Awaiting Review to 5.3

Good points, the string here could be improved upon to be more useful.

I've marked it for 5.3, so we can improve on it for the next release, as we're past the string freeze for the current release cycle.

@mukesh27
4 months ago

Updated text.

#2 @mukesh27
4 months ago

  • Keywords has-patch added

@birgire and @Clorith Please suggest text if needed.

@subrataemfluence
4 months ago

Added suggested link against text "Site Health Status page".

#3 @subrataemfluence
4 months ago

@mukesh27 I have added the suggested link. Also a related thought (since the text link was suggested). As the page can turn into a long one when all or some sections are expanded, how about adding the same link at the bottom of the page?

#4 follow-up: @birgire
4 months ago

Thanks @mukesh27 and @subrataemfluence for the patches.

In 46946.diff does the part:

If any improvement needed then it will show on the ...

need an "is" after the "improvement" ? (I'm not a native English speaker).

Regarding the link in In 46946-2.diff, I would suggest using:

esc_url( admin_url( 'site-health.php' ) )

instead of

esc_url( admin_url()  . 'site-health.php' )

to e.g. make the path accessible in the admin_url filter.

Also a related thought (since the text link was suggested). As the page can turn into a long one when all or some sections are expanded, how about adding the same link at the bottom of the page?

Thanks for the suggestion on @subrataemfluence. Maybe this consideration would be better focused in a possible new ticket? I guess other admin pages will reuse this design in the future. How to help navigate long pages, in this new design, is a good question.

Last edited 4 months ago by birgire (previous) (diff)

@subrataemfluence
4 months ago

Updated patch. I also excluded the word "page" from the link as per the original ticket description.

#5 in reply to: ↑ 4 @subrataemfluence
4 months ago

Thank you @birgire. As per your advice I have created a new ticket #46956 with the following enhancement request.

Related #46956.

Thanks for the suggestion on @subrataemfluence. Maybe this consideration would be better focused in a possible new ticket? I guess other admin pages will reuse this design in the future. How to help navigate long pages, in this new design, is a good question.

#6 @mukesh27
4 months ago

Patch 46946-3.diff looks good for me

#7 @birgire
3 months ago

In 46946-3.diff it seems like __ and printf have to change order.

We might also add a translator comment on %s.

This ticket was mentioned in Slack in #core-php by leogermani. View the logs.


2 months ago

#9 @leogermani
2 months ago

I have updated the patches with:

  • the fix @birgire mentions that __ was misplaced
  • adding a translators comment
  • adding the "is" to the second sentence

I also applied best practices documented at https://codex.wordpress.org/I18n_for_WordPress_Developers#HTML that makes use of wp_kses for extra security.

Version 0, edited 2 months ago by leogermani (next)

@leogermani
2 months ago

#10 follow-up: @birgire
2 months ago

Thank you for the updated patch @leogermani

Regarding core translations, I don't recall wp_kses used, even though it contains HTML.

This came up recently here:

https://core.trac.wordpress.org/ticket/47070#comment:43

and the patch there was committed without wp_kses.

ps: Awesome, I hope you're enjoying WCEU 2019 :-)

Last edited 2 months ago by birgire (previous) (diff)

#11 in reply to: ↑ 10 @leogermani
2 months ago

Replying to birgire:

Regarding core translations, I don't recall wp_kses used, even though it contains HTML.

Yes, me neither. I think this is kind of super paranoic, given all translations gets reviewd and everything. And also the code gets much more ugly and complicated. But I decided to follow what is in the codex. Do you think I should take it out and submit a new patch?

Maybe Ill go to the polyglots team and see what they think :)

This ticket was mentioned in Slack in #core-php by leogermani. View the logs.


2 months ago

#13 follow-ups: @leogermani
2 months ago

In fact, I was wondering if instead of:

printf( __('Here is my <a href="%s">link to the admin</a>'), admin_url() );

We should have (with appropriate translators comment):

printf( __('Here is my %1$slink to the admin%2$s'), '<a href="' . admin_url() . '">', '</a>' );

This would take the burden of HTML out of the translators hand...

This ticket was mentioned in Slack in #polyglots by leogermani. View the logs.


2 months ago

#15 in reply to: ↑ 13 @foack
2 months ago

Replying to leogermani:

In fact, I was wondering if instead of:

printf( __('Here is my <a href="%s">link to the admin</a>'), admin_url() );

We should have (with appropriate translators comment):

printf( __('Here is my %1$slink to the admin%2$s'), '<a href="' . admin_url() . '">', '</a>' );

This would take the burden of HTML out of the translators hand...

I like that approach. Definitely an increase in usability imho.

#16 @antpb
2 months ago

  • Keywords dev-feedback added

#17 @leogermani
2 months ago

So basically we have 3 options on how to localize a string with a HTML link in it and Im going to summarize them here in this comment to make it easier to get feedback on which is the best.

Option 1, basic

printf( __('Here is my <a href="%s">link to the admin</a>'), admin_url() ); `

Option 2, get HTML out of the translatable string

printf( __('Here is my %1$slink to the admin%2$s'), '<a href="' . admin_url() . '">', '</a>' );

Option 3, As suggested in the codex with extra security precautions (https://codex.wordpress.org/I18n_for_WordPress_Developers#HTML)

printf(
    wp_kses(
        __('Here is my <a href="%s">link to admin</a>'),
        ['a' => ['href' => [] ] ]
    ), 
    esc_url( admin_url( 'site-health.php' ) ) );
)

In my opinion, option 3 looks good. Less chance that the HTML gets broken during the translation and, in simple cases such as this example, we could use only %s instead of %1$s, which would make it even simpler.

#18 @desrosj
2 months ago

  • Component changed from Text Changes to Site Health

Moving Site Health tickets into their lovely new home, the Site Health component.

#19 in reply to: ↑ 13 @tobifjellner
2 months ago

Replying to leogermani:

In fact, I was wondering if instead of:

printf( __('Here is my <a href="%s">link to the admin</a>'), admin_url() );

We should have (with appropriate translators comment):

printf( __('Here is my %1$slink to the admin%2$s'), '<a href="' . admin_url() . '">', '</a>' );

This would take the burden of HTML out of the translators hand...

Both ways are totally OK and we see both of them in various places in Core.
But for version 2, it is critically important that a good explanation is given in a comment to translators. (Personally, I prefer number 1, but again: both ways are fully ok)

#20 @birgire
2 months ago

There are both cons/pros to each approach.

The downside I see with option 2, is that the word boundary get's more blurry imho, with %1$slink.

#21 @leogermani
2 months ago

I was going to look to the source to see what is often used but @tobifjellner already did it, so great!

I had a quick chat with @nao to get an input from the translators and she also said both are fine but that maybe the first option, with the HTML inside, looks cleaner.

I will update the patch with this approach.

As for the documentation in the Codex suggesting to use wp_kses, I have an impression that this is a little too much. If we are worried that malicious HTML could be inserted by a translation, we would have to filter all strings, not only those that have a link, because the malicious code could be inserted in any of them.

I will have a look if that page was already merged to the new Developer Handbook and, if not, I will do so and suggest a change to that recommendation.

Thank you all for the inputs.

@leogermani
2 months ago

#22 @leogermani
2 months ago

  • Keywords dev-feedback removed

#23 @birgire
2 months ago

  • Owner set to leogermani
  • Status changed from new to assigned

Thank you for the updated patch @leogermani

The core translations are considered safe (see e.g. here) so 46946-5.diff looks good to me.

The old Codex pages might not been updated recently.

To hopefully land this in the next milestone, it's required for tickets to have an owner, so I mark you as an owner here @leogermani, if you don't mind :-)

#24 @SergeyBiryukov
12 days ago

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

In 45791:

Site Health: Simplify the language in Status Health Info page introduction, add a link to Status page.

Props birgire, mukesh27, subrataemfluence, leogermani.
Fixes #46946.

Note: See TracTickets for help on using tickets.