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.
to test:
- install (in english)
- install language module and interface translation module
- visit admin/reports/translations
- add a couple languages
- visit admin/reports/translations again
also,
- try installing in another language, and then
- visit admin/reports/translations
- add a couple languages
- visit admin/reports/translations again
Comment | File | Size | Author |
---|---|---|---|
#61 | locale-1978926-61.patch | 23.59 KB | tim.plunkett |
#61 | interdiff.txt | 1.65 KB | tim.plunkett |
#59 | interdiff.txt | 8.02 KB | kim.pepper |
#59 | 1978926-local-translation-status-59.patch | 22.14 KB | kim.pepper |
#56 | interdiff.txt | 11.05 KB | kim.pepper |
Comments
Comment #1
PanchoQuite straightforward, but I didn't get to testing it that much.
So let's see what the bot says and then please review.
Comment #2
PanchoSorry, the first patch was missing our new class. No other changes.
Here we go again:
Comment #3
PanchoComment #5
PanchoNew patch:
Now let's see what the testbot says.
In any case still open for further improvements, optimizations and suggestions.
Comment #7
Pancho#5: locale_translation_status_form_controller-1978926-5.patch queued for re-testing.
Previous result: FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
Comment #9
h3rj4n CreditAttribution: h3rj4n commentedtaking over the issue.
Comment #10
h3rj4n CreditAttribution: h3rj4n commentedThis patch is definitely going to fail but the old one couldn't be applied, so just created a new one.
Comment #11
h3rj4n CreditAttribution: h3rj4n commentedOk, I hope this one doesn't fail. Should all be fixed.
Comment #12
h3rj4n CreditAttribution: h3rj4n commentedForgot 'needs review' again :S
Comment #14
h3rj4n CreditAttribution: h3rj4n commentedquick update: The locale module requires the update module to be enabled. Otherwise this module doesn't work. It's about this function
locale_translation_build_projects
. It works but won't give you the functionality you want.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
Gábor HojtsyComment #18
disasm CreditAttribution: disasm commentedmassive reroll. Built on FormBase. Injecting KeyVaueStoreInterface to get rid of Drupal::state(). Compared functions being replaced with methods and made changes accordingly. Not throwing any errors locally. Lets see what testbot thinks!
Comment #19
disasm CreditAttribution: disasm commentedNoting that if anything looks odd in the interdiff (like removing locale_translation_manual_status), that isn't really happening. It was a screw-up I made in the reroll somehow that I fixed in my commit (in the reroll I ended up with two locale_translation_manual_status functions, one of which contained the stuff I really wanted to remove.
Comment #21
Luxian@disasm Your patch looks really good and I would like to continue your work and try to get all the Locale forms converted. The other forms already have a working patch:
So, I will assign this to me.
Comment #22
LuxianThis is the patch from #1978926-18: Convert locale_translation_status_form to a Controller re-rolled with conflicts resolved. I have 3 tests failing on my local. I'm not sure how much time I will have in the next 2 days, so if anyone wants to work on it can assign it to himself.
Interdiff might be wrong (not sure if I do them right).
Comment #24
artem.ua CreditAttribution: artem.ua commented22: drupal.locale.update_form_status-1978926-22.patch queued for re-testing.
Comment #26
neetu morwani CreditAttribution: neetu morwani commentedLast patch re-rolled. Now the patch applies successfully.
Comment #28
victor-shelepen CreditAttribution: victor-shelepen commentedComment #30
victor-shelepen CreditAttribution: victor-shelepen commentedComment #32
ehegedus CreditAttribution: ehegedus commentedIt still applies, tag removed.
Comment #33
victor-shelepen CreditAttribution: victor-shelepen commentedif it is a class. It always have to have an attribute.
Comment #34
YesCT CreditAttribution: YesCT commentedwas going to review this, but it did not apply.
rerolled.
easy, auto 3-way merge.
no conflicts. (no interdiff)
I'll review this now.
Comment #35
tim.plunkettBad merge? It's supposed to be removing the other one.
Comment #36
victor-shelepen CreditAttribution: victor-shelepen commented@tim.plunkett This is from
https://drupal.org/node/1978924. It is a pitty that it has been closed automaticaly due to no activity. We need an authorized person to solve such problems. There are a lot rtbc tasks.
Comment #37
victor-shelepen CreditAttribution: victor-shelepen commentedAfter this issue and the issue https://drupal.org/node/1978918 are commited we need remove a file that realizes \Drupal\locale\Form\LocaleForm because its unuseless. So we need commit at the first.
Comment #38
victor-shelepen CreditAttribution: victor-shelepen commentedhttps://drupal.org/node/2249449
Comment #40
victor-shelepen CreditAttribution: victor-shelepen commentedComment #41
victor-shelepen CreditAttribution: victor-shelepen commentedI've corrected variable definitions.
Comment #42
YesCT CreditAttribution: YesCT commented1.
we dont use inheritdoc on classes.
fixing that.
--------notes
at first I tried something like find . -name "*.php" | xargs awk '/inheritdoc/,/function/' > /tmp/results.txt
but could not get that to work, so instead did a bit of manual work and did
ag inheritdoc -A 2 > /tmp/results.txt
then
ag class -A 2 /tmp/results.txt
and looked through those results.
#2250165: Replace fake mocks with actual OpenDialogCommand stubs in AjaxCommandsTest opened to take care of that.
--------end notes
2.
also fixing nit of missing newline after open { of the class.
"Leave an empty line between start of class/interface definition and property/method definition:"
https://drupal.org/node/608152#indenting
3.
either this issue, or a separate issue needs to add tests for this form/table.
because with this patch, the table at admin/reports/translations
should show
but it shows
Comment #44
YesCT CreditAttribution: YesCT commented42: locale-update_status_form-1978926-42.patch queued for re-testing.
Comment #45
victor-shelepen CreditAttribution: victor-shelepen commented42: locale-update_status_form-1978926-42.patch queued for re-testing.
Comment #46
victor-shelepen CreditAttribution: victor-shelepen commentedI've corrected the dunctionality according to snapshots #42, removed extra code according to #35. The test is defined for but it does not cover on fully Drupal\locale\Tests\LocaleUpdateTest::testUpdateCheckStatus()
I tried
But it crashes tests because because this page caches some settings.
Comment #47
victor-shelepen CreditAttribution: victor-shelepen commentedComment #48
YesCT CreditAttribution: YesCT commentedI haven't checked for all the forms this effects and tried them all manually. We might still want to do that.
But there was a line in #47 I didn't understand why was added.
So, what I did was take 42, and add the test (from 47, but only part of 47) and take out the export wrapper.
This had 1 fail (agreeing with 42 3.) Good! Uploading that to show the test works (fails).
Then I added the fix from #46.
I talked with @likin in irc, but I still dont understand #46
Maybe #2264027: Refactoring locale_translation_get_status will clarify?
taking the needs tests tag off since the only problem spotted so far, has had a assert added for it.
We might want to keep an eye on this, as in the future, when adding a language, core should be able to get the translation automatically. But we dont need to worry about that here, because now we have a test that will fail when that does begin to work, and that issue can update this test.
Comment #50
victor-shelepen CreditAttribution: victor-shelepen commentedThe patch from the comment 47 has passed tests succussfuly. So It was a temporary fail. We can close the task #2264027: Refactoring locale_translation_get_status. I hope this task is ready to be commited. :) Tasks appear these modify the logic. So we need finish the convertion and continue the logic modification later.
Comment #51
penyaskitoAre these included in any other forms? Should we make those services? I guess old-style includes are not good, but this can be done in follow-up.
Otherwise, looks good. I will proceed with manual testing if noone beats me to do it.
Comment #52
victor-shelepen CreditAttribution: victor-shelepen commentedIt is modified according to #51
Comment #53
rodrigoaguileraI've done the manual testing according to the issue summary and everything looks alright to me.
Comment #54
rodrigoaguileraComment #56
kim.pepperI've re-rolled for PSR4 and made some code-style clean ups.
This issue is pretty old, so there may be a few other cleanups needed.
Tests are passing locally.
Comment #57
kim.pepper56: 1978926-local-translation-status-56.patch queued for re-testing.
Comment #58
tim.plunkettIs *any* of this in scope?
Otherwise the rest looks good.
Comment #59
kim.pepperSorry! Got a bit excited with reformatting.
This reverts code style cleanups @tim.plunkett mentions in #58
Comment #60
tim.plunkettExcellent, thanks!
Comment #61
tim.plunkettActually, too soon. Deleted some stale code.
Comment #62
jibranBack to RTBC
Comment #63
Gábor HojtsyYay, thanks!
Comment #66
tim.plunkettEntityCrudHookTest has been randomly failing all day :(
Comment #67
alexpottCommitted 8b45d21 and pushed to 8.x. Thanks!
Minor fixes on commit.
Comment #69
Gábor HojtsyYay, thanks!