Problem/Motivation

locale_config_batch_set_config_langcodes() and locale_config_batch_refresh_name() are batch operation callbacks which are just wrappers around API methods on the LocaleConfigManager.

However, the names don't match up at all which is confusing:

function locale_config_batch_set_config_langcodes(&$context) {
  Locale::config()->updateDefaultConfigLangcodes();

and:

function locale_config_batch_refresh_name(array $names, array $langcodes, &$context) {
  SNIP
  $context['results']['stats']['config'] += Locale::config()->updateConfigTranslations($names, $langcodes);

Steps to reproduce

Proposed resolution

Rename both callbacks to 'locale_config_batch_' + snake case version of the API method it calls.

No BC handling is necessary as batch API callbacks are internal.

Remaining tasks

  • Update the merge request to retain the procedural functions that wrap the new methods, and add a deprecation (see #5).
  • Fix the coding standards error in the pipeline job (see #16).

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#33 3422977-nr-bot.txt1.69 KBneeds-review-queue-bot
#20 3422977-nr-bot.txt1.62 MBneeds-review-queue-bot

Issue fork drupal-3422977

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

joachim created an issue. See original summary.

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

shalini_jha opened merge request !6742

joachim’s picture

joachim commented
Status: Active » Reviewed & tested by the community

Thanks for the MR. Looks goo!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

These are procedural functions without a leading underscore, we should leave the old one as wrappers and deprecate them.

prabuela’s picture

prabuela commented
Assigned: Unassigned » prabuela
prabuela’s picture

prabuela commented
Assigned: prabuela » Unassigned
secretsayan’s picture

secretsayan commented
Assigned: Unassigned » secretsayan

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

anicoto’s picture

anicoto
Primary language Spanish
Location Houston, TX
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Portland2024, +backend
Novice issue reserved for the Mentored Contribution during the Contribution Day at DrupalCon Portland 2024. After the 8th of May 2024, this issue returns to being open to all. Thanks
joachim’s picture

joachim commented

Someone should rebase the branch before a novice works on it -- it looks like it's been merged rather than rebased, which is going to make the history messy.

traviscarden’s picture

traviscarden commented

Actually, I think it just conflicts with #3422915: Deprecate Drupal\locale\Locale and its one method. I'll rebase it onto the commit right before that, but someone will need to advise on the new direction: Does the new scope become resolving the conflict? Or is this no longer a novice issue--or even obsolete?

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

MR currently have some phpcs errors.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Novice

I agree with #11 and #12; this is not Novice for right now. Thanks!

joachim’s picture

joachim commented
Issue tags: +Novice

Rebased on 11.x and resolved the conflict.

This is good for novices again -- see #5 for what needs doing.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Agreed, thanks @joachim!

There is also a coding standards issue to resolve:

--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 16 | ERROR | [x] Opening brace should be on the same line as the declaration
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
Time: 16.64 secs; Memory: 8MB
PHP CODE SNIFFER REPORT SUMMARY
-------------------------------------------------------------------------------
secretsayan’s picture

secretsayan commented
Assigned: secretsayan » Unassigned
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left some comments on the MR.

secretsayan’s picture

secretsayan commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3422977-nr-bot.txt1.62 MB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

secretsayan’s picture

secretsayan commented
Status: Needs work » Needs review

Marking this again as Needs Review as PHPSTAN is already passing in the pipeline.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs change record

So function renaming should have a change record and deprecations should point to that vs the ticket.

Thanks

joachim’s picture

joachim commented
Status: Needs work » Needs review
Issue tags: -Needs change record

These are internal functions. They're batch API callbacks, they're not going to be used anywhere else.

smustgrave’s picture

smustgrave commented

Personally still think a CR should be made so will leave for additional reviewers.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Deprecations will have to be updated for deprecated in 11 removed in 12.

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

nexusnovaz’s picture

nexusnovaz commented
Status: Needs work » Needs review

Updated deprecations to be 11.1.0. Could you please review MR !6742. Thanks!

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thanks. I should of caught this before so my bad. But we will need a change record to announce the deprecation. And the links in the deprecation needs to be with that CR link.

CR can be pretty simple, would look through existing ones in the change records list.

nexusnovaz’s picture

nexusnovaz commented
Status: Needs work » Needs review

Hey, made the CR which can be found here: https://www.drupal.org/node/3475054. Also replaced the issue links in the MR with this link. Please can this be reviewed again.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks! Believe this one is good

traviscarden’s picture

traviscarden commented
Title: rename locale batch operation callbacks to match the API methods they call » Rename locale batch operation callbacks to match the API methods they call
quietone’s picture

quietone commented

I read the issue summary, comments and the MR. Everything looks in order here. The deprecation messages in the docblocks don't need to list the function arguments but it is also not wrong. So, leaving that as is. And thanks for the nicely formatted change record, it is is easy to read.
I updated credit.

Leaving at RTBC.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3422977-nr-bot.txt1.69 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

sayan_k_dutta’s picture

sayan_k_dutta commented
Assigned: Unassigned » sayan_k_dutta

Working on it.

traviscarden’s picture

traviscarden commented
Assigned: sayan_k_dutta » Unassigned
Status: Needs work » Needs review

@secretsayan asked me to push this change for him real quick because he's in the process of moving to a new laptop.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Failure seems unrelated, re-running. Resolved the threads while I was at it. Bot feedback appears addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 11.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 6b6facdc29e to 11.x and 167dc216f33 to 11.1.x. Thanks!

alexpott’s picture

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

  • alexpott committed 167dc216 on 11.1.x 
    Issue #3422977 by secretsayan, traviscarden, nexusnovaz, joachim,...

  • alexpott committed 6b6facdc on 11.x 
    Issue #3422977 by secretsayan, traviscarden, nexusnovaz, joachim,...

  • alexpott committed b5a06149 on 11.1.x 
    Issue #3422977 follow-up by alexpott: Rename locale batch operation...

  • alexpott committed 151ef182 on 11.x 
    Issue #3422977 follow-up by alexpott: Rename locale batch operation...
traviscarden’s picture

traviscarden commented

Congratulations on your first Core commit, @secretsayan! 😄

secretsayan’s picture

secretsayan commented

Thanks @traviscarden :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

xjm
she/her
Primary language English
commented

Amending attribution.