Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 22 months ago

#54437 closed defect (bug) (fixed)

Warning not in bold?

Reported by: nekojonez's profile NekoJonez Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: trivial Version:
Component: General Keywords: good-first-bug has-patch has-unit-tests i18n-change
Focuses: ui, administration, template, coding-standards Cc:

Description (last modified by SergeyBiryukov)

Most of the warnings in WP have the "warning" part in bold. Yet, there are a few that aren't in bold.

Except these:

https://build.trac.wordpress.org/browser/tags/5.9.2/wp-includes/functions.php?marks=1583#L1582
https://build.trac.wordpress.org/browser/tags/5.9.2/wp-includes/class-wp-theme.php?marks=284#L283

For the sake of being consistent, shouldn't these have bold tags as well?

Attachments (1)

54437.diff (1.2 KB) - added by oakesjosh 2 years ago.

Download all attachments as: .zip

Change History (33)

#1 @NekoJonez
2 years ago

  • Version set to 5.8.2

#3 in reply to: ↑ description @NekoJonez
2 years ago

  • Focuses ui template added
  • Type changed from enhancement to defect (bug)
  • Version changed from 5.8.2 to 5.8.3

#4 @SergeyBiryukov
2 years ago

  • Description modified (diff)
  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 6.0
  • Version 5.8.3 deleted

Thanks for the ticket!

Updated the description with permanent links so that the locations do not change. Removing the version, as these messages were not introduced in 5.8.2 or 5.8.3.

It looks like most of the "Error:" prefixes in core are placed inside a <strong> tag, so I think it makes sense to do the same here.

@oakesjosh
2 years ago

#5 @oakesjosh
2 years ago

  • Keywords has-patch added; needs-patch removed

#7 @audrasjb
23 months ago

  • Keywords commit assigned-for-commit added

We reviewed this patch during WordCamp Geneva 2022’s contribution workshop.

The above PR was meant to test the provided patch.
Looks like tests are passing.
Self assigning for commit.

#8 @audrasjb
23 months ago

  • Owner set to audrasjb
  • Resolution set to fixed
  • Status changed from new to closed

In 53118:

General: add missing strong tag to some error messages.

Props NekoJonez, oakesjosh.
Fixes #54437.

#10 @NekoJonez
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm reopening this, since I found another one during translation.
https://build.trac.wordpress.org/browser/trunk/wp-admin/includes/ajax-actions.php?marks=1294#L1294

This ticket was mentioned in PR #2666 on WordPress/wordpress-develop by NekoJonez.


22 months ago
#11

During development of 6.0, a trac ticket of mine was fixed. This was adding strong tags in warnings. Yet, during translation I found two others.

Trac ticket: (https://core.trac.wordpress.org/ticket/54437)

This ticket was mentioned in Slack in #core by nekojonez. View the logs.


22 months ago

NekoJonez commented on PR #2666:


22 months ago
#13

I should have checked the other branches before submitting this PR... Now, I have found another one. All branches have been checked and I think I got them all.

#14 follow-up: @audrasjb
22 months ago

  • Keywords commit removed

Thanks @NekoJonez 👍

#15 in reply to: ↑ 14 @NekoJonez
22 months ago

Replying to audrasjb:

Thanks @NekoJonez 👍

No problem @audrasjb sorry for not checking the other translation branches as well before first submitting the PR... Now, I got them all so I think if no objections in this PR happen it's all fine to go and get this ticket re-fixed :)

audrasjb commented on PR #2666:


22 months ago
#16

@NekoJonez tests are now failing though:

1) Tests_Ajax_ReplytoComment::test_with_draft_post
Failed asserting that exception message '<strong>Error:</strong> You cannot reply to a comment on a draft post.' contains 'Error: You cannot reply to a comment on a draft post.'.

You need to also update unit tests :)

NekoJonez commented on PR #2666:


22 months ago
#17

@NekoJonez tests are now failing though:

1) Tests_Ajax_ReplytoComment::test_with_draft_post
Failed asserting that exception message '<strong>Error:</strong> You cannot reply to a comment on a draft post.' contains 'Error: You cannot reply to a comment on a draft post.'.

You need to also update unit tests :)

As a noob in this, some help with that would be nice.

NekoJonez commented on PR #2666:


22 months ago
#18

@ocean90 , I removed all strong tags in the AJAX php file... And tests are still failing.
Do those strong tags also need to be removed in the other files?

This ticket was mentioned in PR #2669 on WordPress/wordpress-develop by audrasjb.


22 months ago
#19

  • Keywords has-unit-tests added

audrasjb commented on PR #2666:


22 months ago
#20

@NekoJonez I opened another PR to fix the failing unit tests: https://github.com/WordPress/wordpress-develop/pull/2669
Does it look good to you?

NekoJonez commented on PR #2666:


22 months ago
#21

@NekoJonez I opened another PR to fix the failing unit tests: https://github.com/WordPress/wordpress-develop/pull/2669
Does it look good to you?

@audrasjb Excellent. Now I dont have to worry about why my things are failing.

NekoJonez commented on PR #2669:


22 months ago
#23

Excellent! Thank you @audrasjb for this help. I have no idea why for me the unit tests were failing.

#24 @audrasjb
22 months ago

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

In 53337:

General: Add missing strong tag to some error messages.

This changeset adds missing strong tag to some error messages and updates some AJAX messages, for better consistency.

Follow-up to [53118].

Props NekoJonez, audrasjb, ocean90.
Fixes #54437.

#26 @SergeyBiryukov
22 months ago

We're currently in soft string freeze as per the 6.0 release schedule, so no strings should be changed at this point, except for the About page. I have no strong objections to leaving this change in 6.0, but as these strings don't seem new, I think it would have been better to handle this in a separate ticket early in 6.1 instead, same as #55661.

#27 @audrasjb
22 months ago

  • Keywords assigned-for-commit removed

@SergeyBiryukov yes, but it appears this issue was spotted while translating 6.0 strings, so in my understanding – and I might be wrong :) – the current phase of the release cycle is the good timing to fix issues?

#28 @SergeyBiryukov
22 months ago

  • Keywords i18n-change added

I think it would be a good timing if these strings were new in 6.0, or somehow edited earlier in this release cycle, or somehow connected to the strings changed in [53118].

It's probably not much of an issue, just a matter of sticking to our own release schedule and something to keep in mind for future releases to avoid adding unexpected work for translators.

Adding the i18n-change keyword as per Trac Workflow Keywords.

Last edited 22 months ago by SergeyBiryukov (previous) (diff)

#29 @audrasjb
22 months ago

Ok got it, thanks. To clarify: I didn't notice those strings were not new ones, introduced in 6.0 :)

#30 @peterwilsoncc
22 months ago

@SergeyBiryukov, @audrasjb

I'm concerned about leaving [53337] in 6.0 too.

How would it work if this was reverted:

  • have the language packs been updated since RC1?
  • fuzzy matching would pick up the strings from the 5.9 project, would polyglots need to re-approve the old strings?

#31 @audrasjb
22 months ago

@peterwilsoncc it is already declared as a fuzzy string for WP 6.0.
See https://translate.wordpress.org/projects/wp/dev/admin/fr/default/?filters%5Bstatus%5D=either&filters%5Boriginal_id%5D=10187702&filters%5Btranslation_id%5D=75028029 for example.
I'm really sorry for the misundestanding and it is my mistake, but now, reverting the change would add even more work to polyglots teams.

#32 @peterwilsoncc
22 months ago

Don't worry about it too much, we all have misunderstand something at one point or another. I've done similar and worse ;)

Thank for helping my understanding of how fuzzy matching works. It seems best to avoid making things harder for the translation teams.

Note: See TracTickets for help on using tickets.