Problem/Motivation

These will be the final underscore functions.

  • _locale_refresh_translations - public in LocaleJs
  • _locale_refresh_configuration - Inlined in TranslateEditForm
  • _locale_strip_quotes - protected in LocaleJs
  • _locale_parse_js_file - public in LocaleJs
  • _locale_invalidate_js - public in LocaleJs
  • _locale_rebuild_js - public in LocaleJs

Steps to reproduce

Proposed resolution

_locale_rebuild_js should we make this protected and remove the parameter, the only calls outside LocaleJs and with langcode are in tests
_locale_parse_js_file should we make this protected the only calls outside of LocaleJs are in tests
_locale_invalidate_js should become public, it's called by several hooks

Remaining tasks

User interface changes

Introduced terminology

API changes

The following functions have been deprecated:

  • _locale_refresh_translations()
  • _locale_refresh_configuration()
  • _locale_strip_quotes()
  • _locale_parse_js_file
  • _locale_invalidate_js()
  • _locale_rebuild_js()

The following functions have no replacement:

  • _locale_refresh_configuration()
  • _locale_strip_quotes()

The following functions have a replacement:

  • Before: _locale_refresh_translations()
    After: \Drupal::service(Drupal\locale\LocaleJs)->refreshTranslations()
  • Before: _locale_invalidate_js()
    After: \Drupal::service(Drupal\locale\LocaleJs)->invalidate()

The following functions have a replacement, but they are only public for testing purposes. They are marked as internal and can be changed or removed at any time in the future:

  • Before: _locale_parse_js_file()
    After: \Drupal::service(Drupal\locale\LocaleJs)->parseJsFile()
  • Before: _locale_rebuild_js()
    After: \Drupal::service(Drupal\locale\LocaleJs)->rebuild()

Data model changes

Release notes snippet

Issue fork drupal-3618358

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
Title: [pp-1] Deprecate remaining _ functions in locale » Deprecate remaining _ functions in locale
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue tags: +Needs change record updates, +Needs issue summary update

nicxvan opened merge request !16849

nicxvan’s picture

nicxvan commented
Title: Deprecate remaining _ functions in locale » Deprecate remaining underscore functions in locale
nicxvan’s picture

nicxvan commented
Status: Active » Needs review

This is ready for review, the failure seems random: (Drupal\Tests\workspaces\FunctionalJavascript\WorkspacesMediaLibraryIntegration)

nitinkumar_7’s picture

nitinkumar_7 commented

Reviewed the MR. The deprecation shims, CR links, and version metadata all look correct, and the call-site migration is complete and consistent (nice catch avoiding the double-deprecation in refreshTranslations()).

No functional concerns otherwise; the WorkspacesMediaLibraryIntegration failure looks unrelated/flaky as noted.

nitinkumar_7’s picture

nitinkumar_7 commented

Two things before this can move past Needs issue summary update:

parseJsFile() and rebuild() ended up public (with @internal) rather than protected as the issue's Proposed Resolution states, and rebuild() kept the $langcode parameter rather than dropping it. Only stripQuotes() matches the original proposal. Can you confirm this is the intended final approach (I'd guess protected would break direct test coverage) so the issue summary can be updated to match?
The change to locale_string_is_safe()'s @see link (3595086 → 3619103) looks unrelated to this issue's scope -- intentional fix or stray from a rebase?

daffie’s picture

daffie commented
Status: Needs review » Needs work

It looks good.
Just a couple of nitpicks.

nicxvan’s picture

nicxvan commented

Thank you @daffie, I updated one of your suggestions and replied to the two about @internal, let me know if that works!

@nitinkumar_7 thank you for your review:

Can you confirm this is the intended final approach (I'd guess protected would break direct test coverage) so the issue summary can be updated to match?

I was actually hoping for second opinions on this, that is why I left it in the IS and did it this way, it's also why I double marked them.

The change to locale_string_is_safe()'s @see link (3595086 → 3619103) looks unrelated to this issue's scope -- intentional fix or stray from a rebase?

Yes, this was a correction for the CR the link is wrong. It's actually already fixed on 11.x, but we need to update main.

nitinkumar_7’s picture

nitinkumar_7 commented

Thanks for the clarification!

That makes sense to me. I’m fine with parseJsFile() and rebuild() remaining public with @internal if the intent is to preserve the direct test coverage, rather than changing them to protected as originally proposed.

And thanks for clarifying the locale_string_is_safe() @see change if it’s correcting the CR link and bringing main in line with the already-fixed 11.x version, that sounds intentional and appropriate.

With those points clarified, I have no further concerns from my side.

daffie’s picture

daffie commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record updates, -Needs issue summary update

I have updated the IS and the CR.
All my remarks on the PR have been solved.
It all looks good to me.
For me it is RTBC.

  • catch committed 1ae585cd on main 
    task: #3618358 Deprecate remaining underscore functions in locale

By:...

  • catch committed ee8c2fac on 11.x 
    task: #3618358 Deprecate remaining underscore functions in locale

By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

This looks fine to me. We already have an issue open to rework js parsing and this will hopefully make that a bit easier if anything.

Committed/pushed to main and cherry-picked 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.

Status: Fixed » Closed (fixed)

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