Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#67 | locale-import_controller-1978918-67.patch | 12.61 KB | victor-shelepen |
Comments
Comment #1
PanchoThe conversion patch, tested to some degree, now let's see if our testbot agrees.
Comment #3
PanchoOk, test failed because I made the status message consistent to language_admin_add_form_submit().
Adapting the test, also removing an obsolete '@see' tag and a redundant 'use' statement in locale.bulk.inc
Should be fine now.
[edit:] Please ignore wrong interdiff.
Comment #4
PanchoHere's the correct interdiff for #3.
Comment #5
PanchoNice, it basically seems to work. Now I'm open for improvements.
Adding tags.
Comment #6
dawehnerThis feels wrong.
No need for that so far.
Comment #7
kim.pepper#3: locale_translate_import_form_controller-1978918-3.patch queued for re-testing.
Comment #9
PanchoUhh, on retest there suddenly seem to be lots of errors.
I'm gonna check this again today.
Comment #10
vijaycs85Re-rolling with fixes for #6
Comment #11
PanchoThanks!
Comment #13
vijaycs85#10: 1978918-locale_translate_import_form_controller-10.patch queued for re-testing.
Comment #15
jair CreditAttribution: jair commentedComment #16
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #17
disasm CreditAttribution: disasm commentedProbably won't be RTBC by the 14th, but we'll need a FormBase for this anyways, so here's a go at it. Yes, I know moduleHandler loading files in classes is hacky.
Comment #19
disasm CreditAttribution: disasm commented#17: drupal8.locale-module.1978918-17.patch queued for re-testing.
Comment #21
disasm CreditAttribution: disasm commentedAdding missing use flag. Manual testing of import/export worked fine! This might actually make it...
Comment #23
disasm CreditAttribution: disasm commentedI'm perplexed by the number of failures here. I added the German Language, made some changes, exported, un-did the changes I made, imported and all my changes were there.
Comment #24
jibranSome minor issues.
@param missing from doc. we can use @inheritdoc here.
Can we create a follow up to convert this to OO if there is not already one and write a @todo with issue link here.
Are we planning to make it a library? We should make it a library and add @todo here with issue link.
Comment #25
Gábor HojtsyComment #26
Luxian@disasm It's been awhile since you work on this. I'm at the Prague sprint and going to work on this now.
Comment #27
LuxianI think this is part 2 from the process described in: A faster process for Drupal 8 routing conversions
Did the following:
clean-up in the LocaleForm (remove function import())- forgot to put it in diffI'm not sure if interdiff is correct. All local test pass on my machine, hope test bot will confirm this.
Comment #28
LuxianForgot to set status.
Comment #29
vijaycs85@Luxian, it is empty patch file :)
Comment #30
LuxianSorry for the empty patch. This is the right one
Comment #32
victor-shelepen CreditAttribution: victor-shelepen commentedI've applied the patch, checked the wrong test - ViewEditTest localy. It works.
Comment #33
victor-shelepen CreditAttribution: victor-shelepen commented#30: drupal.locale.update_import_form-1978918-30.patch queued for re-testing.
Comment #34
mcrittenden CreditAttribution: mcrittenden commentedTags
Comment #35
victor-shelepen CreditAttribution: victor-shelepen commentedIt does not need re-rolling. It is applied well.
Comment #36
pfrenssenOrder these alphabetically.
Leave an empty line between start of class definition and property definition.
Write the type first, then the variable name.
I don't think it is necessary to document the return value for form builders. Everybody is supposed to know what this returns.
Put a blank line before the brace that closes a class.
For the rest this looks fine to me. There are a lot of function calls left to procedural code but for none of them I know whether there are better alternatives available already.
Comment #37
Luxian-- wrong: please delete --
Comment #38
LuxianTook me some time, but now the patch is re-rolled and changes from #36 included. I had to do updates to make sure we don't lose the changes done to that form meanwhile (can be seen in interdiff).
Comment #39
LuxianComment #41
LuxianRe-roll
Comment #42
LuxianComment #43
kbasarab CreditAttribution: kbasarab commented41: drupal.locale.update_import_form-1978918-41.patch queued for re-testing.
Comment #45
victor-shelepen CreditAttribution: victor-shelepen commentedReroll, the theme function moved to the render-able array.
Comment #46
andypostshould be 'locale_translate_import_form' also remove old one
should be injected like module handler
deprecated, use getLanguages()
#theme
also better to copy/paste a render from original form
is not needed
Comment #47
victor-shelepen CreditAttribution: victor-shelepen commentedComment #48
andypostold form should be removed
missing extra blank line at the end of the class
?!
Comment #49
victor-shelepen CreditAttribution: victor-shelepen commentedComment #51
victor-shelepen CreditAttribution: victor-shelepen commentedComment #52
penyaskitoI'm sad this does not apply anymore :(
Comment #53
victor-shelepen CreditAttribution: victor-shelepen commentedPatch has been rerolled. Manul testing went not bad. Look problem tests.
Comment #55
victor-shelepen CreditAttribution: victor-shelepen commentedComment #56
andypostPlease upload a patch
Comment #57
victor-shelepen CreditAttribution: victor-shelepen commentedComment #58
dawehnerwe do have a interface for the language manager.
Missing docs + language manager interface
it would be kind of cool to have a one liner why we always reset the language manager here.
Do we have a follow up to convert this into a separate service?
Isn't that js file already a library?
language load is languageManager->getLanguage
we can use $this->t() here.
Comment #59
victor-shelepen CreditAttribution: victor-shelepen commented1,2,3 - Resolved.
4 . It seems a deprecated function. There is not a mark that it will become deprecated. https://api.drupal.org/api/drupal/core!modules!language!language.admin.i...
5. Yes. It works without that code.
6,7 - Resolved.
Comment #60
vijaycs85More updates:
1. Removed the temporary callback for import at core/modules/locale/lib/Drupal/locale/Form/LocaleForm.php
2. Removed procedural form definition and it's submit handlers.
Comment #61
vijaycs85Adding docblock for constructor.
Comment #62
dawehnerThank you. All additional function calls can be done later.
Comment #64
penyaskito61: 1978918-locale_import_form-61.patch queued for re-testing.
Comment #65
penyaskitoBack to RTBC, the bot will turn back to need works if it fails. There has been some error with them, not related with the patch.
Comment #67
victor-shelepen CreditAttribution: victor-shelepen commentedTo fix the last test I've moved the validation logic from submitForm to validationForm.
Comment #68
andypostGreat
Comment #69
penyaskito@likin w00t, thanks! +1 RTBC.
Removed "Needs reroll" tag.
Comment #70
alexpottCommitted 4b172c7 and pushed to 8.x. Thanks!
Can we get an follow up to move
language_admin_predefined_list()
- this was mentioned in #58Comment #72
alexpottCreated #2254495: Move language_admin_predefined_list to ConfigurableLanguageManager