DateHelper::dayOfWeekName() returns untranslated name

Thank you @Pablo_Pukha, this is exactly what we need. I just added fix for test.

MR needs to be update to 11.x vs 11.0.x

11.x is the "main" development branch.

Eugene Bocharov → changed the visibility of the branch 3456417-datehelperdayofweekname-returns-untranslated to hidden.

Opened MR to 11.x

Problem is this could break existing sites that are using this function as it's changing what's returned so will have to think about that one.

Yes, but previous change might have broken sites that used original behavior of the method. Here we just return to original state. It is over the year since last change though. So I'm not sure what's the best solution at this point. There might be broken expectation either way.

I think it needs change record and detail the docs comment about return value

Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies. Also, 10.2 is in security mode now.

See a CR has been made but moving to NW for 2nd part of #13

I've updated doc comment, please review.

Believe feedback to be addressed.

This does look like a regression, changing title. I updated the IS to point to the comment where the change was made in the originating issue.

The change here look correct but I don't like that there is not a test to prevent a regression in the future. Also, the test in the MR passes when \Drupal\Core\Datetime\DateHelper::dayOfWeekName has not been changed. So, I do think we should take the time to add a test so this does not happen in the future.

The change record states that this currently always returns the English but that will not be true for sites that install with a language other than English. Adding tag for CR update.

I went to update the 'remaining tasks' section but it is missing. Restoring that and updating it.

The change record states that this currently always returns the English but that will not be true for sites that install with a language other than English. Adding tag for CR update.

Hm, I can not see how it's possible.
Currently it returns

return $days[$dow]->getUntranslatedString();

But this returns TranslatableMarkup::$string, which is just string passed to the constructor. And both DateHelper::weekDays() and DateHelper::weekDaysAbbr() pass English strings to the t(). Please, correct me if I'm wrong.

Sorry, you are correct. I must have accidentally tested with the MR applied.

Added test of return value type.
Removing "Needs change record updates", see #19, #20

What @quietone says in #18:

So, I do think we should take the time to add a test so this does not happen in the future.

That still sounds correct. Test changing the language eg from default 'en' to 'fr' and asserting that the day of the week in that language is returned. eg 'lundi'.

The concerns about regressions seem reasonable. But the rule is to create a test for the functionality being implemented whethere it is a new features, a reversion to old code. In terms of creating a failing test, those matters are immaterial. The test seems to me to be missing and should be added. It should fail before the MR is applied and pass after it is applied.

If such a test is in place already, then I have misunderstood.

I've added test just for returning value type, because, as for me, checking exact value returning from TranslatableMarkup looks more like test for translating system, which should be done (and obviously done) in another place.

@eugene bocharov I had a look at possible places in core to create the test, looked inside the translation system. I could find no obvious place to put a test whether a unit or other type of test.

I've added test just for returning value type

That seems the right approach.

Test only job fails as expected:
https://git.drupalcode.org/issue/drupal-3456417/-/jobs/3168817

It seems that no more work is needed so we can go further

There are no unanswered question in the comments or the MR. I updated credit.

Thanks, the fix is simple. Backporting this down to 10.3.x so we get the fix in all current versions of core.

Committed and pushed 3aaf8f3b86c to 11.x and 3520ae258a4 to 11.1.x and 567b629dab3 to 11.0.x and 6ad67e89ca2 to 10.5.x and c2ff9ac27f7 to 10.4.x and 0d414462a53 to 10.3.x. Thanks!

longwave → closed merge request !8493