Problem/Motivation

Let's convert and deprecate:

  • locale_translation_use_remote_source - Inline this logic
  • locale_string_is_safe - Move to LocaleXss::stringIsSafe
  • locale_translatable_language_list - Move to LocaleLanguages
  • locale_js_translate - Move to LocaleJs
  • locale_translation_language_table - move to form class
  • locale_is_translatable - Move to LocaleLanguages

Steps to reproduce

Open locale.module

Proposed resolution

Convert them to the classes in the Motivation section

Remaining tasks

Review
Decide if we need to do BC for the constructor changes.

This adds a new parameter or two to many classes, some are hooks or event subscribers which we generally don't need to do BC for.
Some are forms, which we sometimes provide for and a few services.
Here is the list of classes not counting new ones.

  • 5 services
  • 5 forms
  • 5 hook classes
  • 1 event subscriber

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#11 3616277-nr-bot_s1rv59ma.txt667 bytesneeds-review-queue-bot

Issue fork drupal-3616277

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

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Parent issue: » #3215707: [META] Modernize Locale module
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented

I still need to add the constructor deprecations, but let's see how tests look.

nicxvan opened merge request !16691

nicxvan’s picture

nicxvan commented
Status: Active » Needs review

I think this is ready for review now for naming and placement.

I didn't do constructor BC, I'm not sure we need to do it for forms.

There is no contrib extending TranslateFormBase.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
nicxvan’s picture

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

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3616277-nr-bot_s1rv59ma.txt667 bytes

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.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
Issue tags: +no-needs-review-bot

Github is down so the tests are failing but the changes are good for review.

nicxvan’s picture

nicxvan commented
Issue tags: -no-needs-review-bot

Pretty sure there nightwatch test is random.

Fixed a bunch of phpstan issues introduced on the last rename.

Should be good again.

daffie’s picture

daffie commented
Status: Needs review » Needs work
Issue tags: +Needs change record updates, +Needs issue summary update

The PR looks good to me. Almost RTBC. Just a couple of nitpicks.
Could you add in the CR code examples with before and after for what has changed.
The IS needs to be updated for the deprecations.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented

I updated the IS and the CR, thanks, those had gotten out of date.

I also tested both : and :: for the form callback, both work.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
Issue tags: -Needs change record updates, -Needs issue summary update

This should be ready for review again thanks!

nicxvan’s picture

nicxvan commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

See my comment.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Thanks for the clarifications, I've updated it and it's now green again!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me now. locale_translation_use_remote_source() being inlined is currently quite verbose, but that's largely due to the long config and constant. We have a separate issue to convert the constants, the different values of that setting could be an enum, that could already make it significantly more readable.

I contributed quite a bit with specific suggestions on naming and so on, if not actual code, but we also have an "Almost RTBC" in #14 from @daffie, and we answered those nitpicks.

  • catch committed cd459461 on main 
    task: #3616277 Deprecate several locale functions

By: nicxvan
By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Would have been nice if #2607376: Remove on-demand JavaScript translation parsing and do everything on rebuild and #3525743: Locale JavaScript translation doesn't take into account AJAX so we weren't porting the current logic to OOP, but the moved code is explicitly marked @internal so I think it's OK.

For 11.x I'm wondering if we want/need bc for some of the added constructor arguments, but for main we definitely don't so going ahead and committing there.

nicxvan opened merge request !16830

nicxvan’s picture

nicxvan commented
Status: Patch (to be ported) » Needs review

Had a quick discussion in slack about BC with @catch.

I added BC for the services, I searched contrib for the forms and only a couple contrib modules extended any of the forms and none overrode the constructor so we think it's ok to skip BC there.

We generally exclude bc for hooks, and event subscribers.

nicxvan changed the visibility of the branch 3616277-deprecate-several-locale to hidden.

nicxvan’s picture

nicxvan commented

Ok this is ready now.

There was a bit of shuffling since main had already autowired LocaleConfigManager but 11.x hadn't.

I added BC for the 5 services:

  • LocaleFileManager
  • LocaleConfigManager
  • LocaleFetch
  • LocaleProjectChecker
  • LocaleSource

I added autowiring for LocaleConfigManager and locale.config_subscriber
And switched to explicit autowire true instead of ~ for the two new services.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

I think the 11.x backport is OK too. checked the extra commits, the resulting services.yml, CR link seems correct too. Tests before rebase were on HEAD and are now fixed.

catch’s picture

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

Committed/pushed to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed d5f08dcc on 11.x 
    task: #3616277 Deprecate several locale functions

By: nicxvan
By:...

Status: Fixed » Closed (fixed)

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