Problem/Motivation

#2806009: Installing a module causes translations to be overwritten moves the functionality of locale_system_set_config_langcodes() into LocaleConfigManager. This issue will deprecate the old method and replace any remaining usages. It will also have a change record to inform developers.

Proposed resolution

Add deprecation
Remove usages

Remaining tasks

None

User interface changes

None

API changes

locale_system_set_config_langcodes() is deprecated.

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#17 interdiff_14-17.txt2.33 KBpooja saraah
#17 3337900-17.patch4.53 KBpooja saraah
#14 interdiff-12-14.txt546 bytesrassoni
#14 3337900-14.patch4.41 KBrassoni
#12 3337900-12.patch4.42 KBalexpott
#12 6-12-interdiff.txt2.08 KBalexpott
#6 3337900-6.patch2.34 KBalexpott
#6 3-6-interdiff.txt748 bytesalexpott
#5 Screenshot 2023-03-24 at 12.22.31.png54.58 KBricardofaria
#3 3337900-3.patch2.29 KBalexpott

Issue fork drupal-3337900

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

alexpott created an issue. See original summary.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Postponed » Active
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3337900-3.patch2.29 KB

None of the usages in tests are necessary because the default language in these cases is en so it's doing absolutely nothing.

I don't think adding a test to call locale_system_set_config_langcodes () to prove the deprecation is worth it in this case.

Status: Needs review » Needs work

The last submitted patch, 3: 3337900-3.patch, failed testing. View results

ricardofaria’s picture

ricardofaria commented
StatusFileSize
new Screenshot 2023-03-24 at 12.22.31.png54.58 KB

I can confirm that the patch on #3 is working.
Applying both patches(parent and this one), it works.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 3-6-interdiff.txt748 bytes
new 3337900-6.patch2.34 KB

Fixed the test I broke. It's good that \Drupal\Tests\locale\Kernel\LocaleConfigSubscriberForeignTest exists - unfortunately not real enough to have discovered #2806009: Installing a module causes translations to be overwritten but at least the inner workings of LocaleConfigSubscriber are tested with a different default language.

@ricardofaria thank you for looking into this issue.

Posting screenshots of your codebase or command-line interface does not advance the issue, since the automated testing infrastructure tells us whether the change set still applies correctly.

So, I have not granted issue credit for that screenshot. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose. Thank you!

Status: Needs review » Needs work

The last submitted patch, 6: 3337900-6.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

Failure is unrelated but I'm curious why UserMailNotifyTest.php does not need replacement?

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +@deprecated

Not sure it needs deprecation test

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@andypost UserMailNotifyTest doesn't have a different default language. This method only does something when that is true. UserMailNotifyTest adds other languages but it does not set them as a default.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Can we add a test case for the deprecation message.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 6-12-interdiff.txt2.08 KB
new 3337900-12.patch4.42 KB

I'm not convinced that a test is truly necessary but /shrug here one is - and at least it tests the functionality. Testing the message is probably the least important part - testing that we have not broke the functionality (and don't while the method is still around) is the one reason in favour of adding tests like this.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Gotcha. Been under the mindset that deprecation should always have tests. Got a good rule of thumb for when that's not the case?

Build errors on the patch.

rassoni’s picture

rassoni commented
Status: Needs work » Needs review
StatusFileSize
new 3337900-14.patch4.41 KB
new interdiff-12-14.txt546 bytes

Fixed CCF failed.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

It's 10.2 target, makes sense to update patch

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

pooja saraah’s picture

pooja saraah commented
StatusFileSize
new 3337900-17.patch4.53 KB
new interdiff_14-17.txt2.33 KB

Replaced the deprecated function call locale_system_set_config_langcodes() with the replacement \Drupal\locale\LocaleConfigManager::updateDefaultConfigLangcodes()
These changes should ensure compatibility with Drupal 11.0.0 while addressing the deprecation warnings.
Attached patch against Drupal 11 and interdiff file.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Please open mr and deprecation should ne in 10.3.0

andypost’s picture

andypost
he/him
Primary language Russian
commented

Please open mr and deprecation should ne in 10.3.0

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs reroll

prem suthar made their first commit to this issue’s fork.

prem suthar changed the visibility of the branch 3337900- to hidden.

prem suthar opened merge request !11274

prem suthar’s picture

prem suthar commented

Re-rolled the patch As per #20 and as per the #19 updated the version from 10.3.1 to 10.3.0.

prem suthar’s picture

prem suthar commented
Status: Needs work » Needs review

Please review the MR !11274 .

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left comments on the MR

@prem suthar thanks for rerolling but typically is advised when rerolling to also review the MR too as things or numbers need to be updated.

Thanks.

prem suthar’s picture

prem suthar commented
Status: Needs work » Needs review

@smustgrave please review the changes .

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Still has open threads.

rpayanm made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.