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
| Comment | File | Size | Author |
|---|
Issue fork drupal-3616277
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:
Comments
Comment #2
nicxvan commentedComment #3
nicxvan commentedComment #4
nicxvan commentedI still need to add the constructor deprecations, but let's see how tests look.
Comment #6
nicxvan commentedI 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.
Comment #7
nicxvan commentedComment #8
nicxvan commentedComment #9
berdirComment #10
nicxvan commentedComment #11
needs-review-queue-bot commentedThe 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.
Comment #12
nicxvan commentedGithub is down so the tests are failing but the changes are good for review.
Comment #13
nicxvan commentedPretty sure there nightwatch test is random.
Fixed a bunch of phpstan issues introduced on the last rename.
Should be good again.
Comment #14
daffie commentedThe 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.
Comment #15
nicxvan commentedComment #16
nicxvan commentedI updated the IS and the CR, thanks, those had gotten out of date.
I also tested both
:and::for the form callback, both work.Comment #17
nicxvan commentedThis should be ready for review again thanks!
Comment #18
nicxvan commentedComment #19
berdirSee my comment.
Comment #20
nicxvan commentedThanks for the clarifications, I've updated it and it's now green again!
Comment #21
berdirThis 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.