Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#46946 closed enhancement (fixed)

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

Reported by: birgire's profile birgire Owned by: leogermani's profile 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 5 years ago.
Updated text.
46946-2.diff (1.0 KB) - added by subrataemfluence 5 years ago.
Added suggested link against text "Site Health Status page".
46946-3.diff (1.0 KB) - added by subrataemfluence 5 years 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 5 years ago.
46946-5.diff (1.1 KB) - added by leogermani 5 years ago.

Download all attachments as: .zip

Change History (29)

#1 @Clorith
5 years 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
5 years ago

Updated text.

#2 @mukesh27
5 years ago

  • Keywords has-patch added

@birgire and @Clorith Please suggest text if needed.

@subrataemfluence
5 years ago

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

#3 @subrataemfluence
5 years 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
5 years 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" ? Maybe plural instead? (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.

Version 1, edited 5 years ago by birgire (previous) (next) (diff)

@subrataemfluence
5 years ago

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

#5 in reply to: ↑ 4 @subrataemfluence
5 years 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
5 years ago

Patch 46946-3.diff looks good for me

#7 @birgire
5 years 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.


5 years ago

#9 @leogermani
5 years 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.

( patch created during the WCEU 2019 Dev Day :) )

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

@leogermani
5 years ago

#10 follow-up: @birgire
5 years 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 5 years ago by birgire (previous) (diff)

#11 in reply to: ↑ 10 @leogermani
5 years 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.


5 years ago

#13 follow-ups: @leogermani
5 years 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.


5 years ago

#15 in reply to: ↑ 13 @foack
5 years 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
5 years ago

  • Keywords dev-feedback added

#17 @leogermani
5 years 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
5 years 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
5 years 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
5 years 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
5 years 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
5 years ago

#22 @leogermani
5 years ago

  • Keywords dev-feedback removed

#23 @birgire
5 years 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
5 years 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.