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.
Remaining tasks
None.
Comment | File | Size | Author |
---|---|---|---|
#131 | language-1978916-131.patch | 8.59 KB | googletorp |
#131 | interdiff.txt | 5.42 KB | googletorp |
#128 | interdiff-patches.124-128.interdiff.txt | 1.18 KB | penyaskito |
#128 | language-1978916-128.patch | 4.23 KB | penyaskito |
#124 | language-1978916-124.patch | 4.19 KB | tim.plunkett |
Comments
Comment #1
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #2
Gábor HojtsyComment #3
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #5
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #7
Gábor HojtsyShort extract from our IRC discussion:
Comment #8
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #10
Dave.Ingram CreditAttribution: Dave.Ingram commented#8: 1978916-convert_locale_translate_page.patch queued for re-testing.
Comment #11
Gábor HojtsyLooks to me like a good straightforward conversion. The routing, classes, etc. look fine. I did not compare the moved lines one by one, assuming based on the prior error we found it was copied over with the new code. Looks good to me!
Comment #12
tim.plunkettThe patch no longer applies. Also here is some feedback.
There should be no blank line after the beginning of a method. I didn't bother highlighting them all, but please go through each one.
Node is not in the use statements, which means this form does not have proper test coverage. This would cause an error.
While fixing it, please typehint with NodeInterface
This should have stat injected.
use Drupal\Component\Utility\String::checkPlain()
Please split this out on multiple lines for readability.
Could these be moved to methods somewhere?
It looks like these are only used by the forms, they should be protected methods.
Once this is moved into the form, it can use the Request object directly.
The consensus so far in #1995620: [policy, no patch] Document how to handle routes for MENU_DEFAULT_LOCAL_TASK has been to *not* define duplicate routes. You should be able to remove this.
Comment #13
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #15
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #17
vijaycs85#15: 1978916-convert_locale_translate_page.patch queued for re-testing.
Comment #19
dawehnerComment #20
victor-shelepen CreditAttribution: victor-shelepen commentedThe rerolled patch #15.
Comment #21
victor-shelepen CreditAttribution: victor-shelepen commentedThe rerolled patch is too old. The system does not work with it. I've recovered the code. It works. It needs code review.
Comment #22
victor-shelepen CreditAttribution: victor-shelepen commented* What is better to use: String::checkPlain or check_plaine?
* I've made a
in TranslateFormBase. I see. It should be decoupled. Where is a right place to get it from service?
* Frequently We use
It is too long. What style should we bend on?
Comment #23
tim.plunkettThis is backwards, we should never use check_plain() inside a class.
This can be injected, see the conversion guide linked at the top of the issue
You already have the $request being passed in.
Comment #24
victor-shelepen CreditAttribution: victor-shelepen commentedThe whole patch.
Comment #25
tim.plunkettMy previous review still stands.
If the constructors of these forms ever change, we'll have to come back here and do it as well.
Instead, let's make this controller ContainerAware (or at least store $this->container), and rewrite this as
And make those two forms implement ControllerInterface, and get the state themselves.
Where is $node ever used?
Once this is converted to ControllerInterface, it can inject this as well.
Use the $request instead
Why not have different submit methods for each button?
Comment #26
YesCT CreditAttribution: YesCT commented@ayelet_Cr It's been a while, are you ok with us helping here?
We should go through each point of the recent reviews and make sure they are addressed.
We dont have to do it all at once, but if we only address a couple, we should summarize and say what we addressed and what is still left to do.
Also, at this point, if possible, we should be using interdiffs each time we post a new patch.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff | Microbranching workflow: http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...
@likin Do you want to try another patch and I can review it? Or do you want to do it the other way around? I can give it a try to do some of the concerns identified. This looks really interesting.
since the patch applies, removing the needs reroll tag. it's still needs work though. :)
Comment #27
victor-shelepen CreditAttribution: victor-shelepen commentedThe reroll process has been done. The notices above have not been applied yet.
Comment #28
victor-shelepen CreditAttribution: victor-shelepen commentedComment #30
tim.plunkett#27: locale-convert_locale_translate_page-1978916-27.patch queued for re-testing.
Comment #32
victor-shelepen CreditAttribution: victor-shelepen commentedI see we have a lot of test problems. Those are not related with locale_traslate_page.
Comment #33
vijaycs85Just a re-roll with current code base and will check what testbot says.
Comment #34
vijaycs85Comment #36
vijaycs85Comment #37
victor-shelepen CreditAttribution: victor-shelepen commented@vijaycs85 Attach interdiff.
Comment #38
dawehnerLet's remove this unneeded whitespace.
Just to nitpick as well: Let's drop 'Page callback: ' as well as add an empty line before the last to closing brackets at the end of the file.
t() should be now be done by using an injected translation service.
It seems to be that we should probably inject this service into the class instead of directly calling it.
Instead of using $_GET['query'] you should always try to use the request object
Comment #39
vijaycs85Thanks for the review @dawehner. Fixed all items in #38
Comment #40
YesCT CreditAttribution: YesCT commentedI'm also looking at this one. I'm going through the previous feedback trying to make sure it was addressed.
Comment #41
YesCT CreditAttribution: YesCT commentedshould this be $locale_storage? maybe $translation_storage?
Comment #43
YesCT CreditAttribution: YesCT commentedHere are diffs of functions that have moved.
I have a patch coming with some cleanup.
Comment #44
YesCT CreditAttribution: YesCT commentedThis
adds doc blocks to classes,
uses \FullyQualified class name in comments "If you use a namespace in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash)." https://drupal.org/coding-standards/docs#classes ,
adds missing @param's and @returns
adds the empty line before the close brace of classes (mentioned in #38 and also shown in https://drupal.org/node/1353118 [edit: oh! and specified here: https://drupal.org/node/608152#indenting in the OO standards]
puts new use statements in alphabetical order
takes out some extra newlines
makes oneline summaries less than 80 chars "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions (noted in the Tag Reference below)." https://drupal.org/coding-standards/docs#drupal
corrects some spelling and grammar (in new comments).
===
some other fixing coming.
Comment #45
YesCT CreditAttribution: YesCT commentedThis snuck into #33
Taking it out.
If it is needed, just let us know. :)
--
more fixes coming.
Comment #47
YesCT CreditAttribution: YesCT commentedarrays when split over multiple lines are usually split with the closing paren on its own line.
@tim.plunkett asked in #12 for it to be split. Just fixing the formatting here. https://drupal.org/coding-standards#array
in #12, tim asked:
+++ b/core/modules/locale/locale.moduleundefined
@@ -1355,3 +1353,135 @@ function _locale_rebuild_js($langcode = NULL) {
+function locale_translate_filter_load_strings() {
...
+function locale_translate_filter_values() {
...
+function locale_translate_filters() {
It looks like these are only used by the forms, they should be protected methods.
they are methods now, but not noted as public or protected.
I made them protected.
Also, https://drupal.org/node/608152#indenting "All methods and properties of classes must specify their visibility: public, protected, or private."
in #12, tim asked for:
+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,232 @@
+ _locale_refresh_translations(array($langcode), $updated);
+ _locale_refresh_configuration(array($langcode), $updated);
Could these be moved to methods somewhere?
seems like those lines were moved from locale_translate_edit_form_submit() to TranslateEditForm::submitForm
I'm not sure that is what was being asked for.
I think tim is asking to move them from locale.module to methods in a class.
=======
I was going back through the history here,
Note #13 doubled in size mostly because of fixing tests... like
> - $this->drupalGet('admin/config/regional/translate/translate');
> + $this->drupalGet('admin/config/regional/translate');
=======
@likin
the reroll in #20
did:
locale_storage()->
to
$this->container->get('locale.storage')->
locale_storage()->
to
Drupal::service('locale.storage')->
language()->langcode
to
language()->id
by:
< $js_strings = locale_storage()->getStrings(array('type' => 'javascript'));
---
> $js_strings = $this->container->get('locale.storage')->getStrings(array('type' => 'javascript'));
just context lines change.
and:
@@ -12,463 +12,6 @@
1128c1128
< - return locale_storage()->getTranslations($conditions, $options);
---
> - return Drupal::service('locale.storage')->getTranslations($conditions, $options);
etc.
but it only changed the context and deleted lines,
it did not then also update those lines in the places those lines were moved to.
so the return in translateFilterLoadStrings needed to be updated to be
return Drupal::service('locale.storage')->getTranslations($conditions, $options);
this is fixed by #39 and is fine now I think.
Mentioning it here to help future rerolls.
Also, in my head, rerolls are when a patch does not apply, and only changes made to the patch are those needed because head has changed. Most often rerolls are difficult to make interdiffs for, but it's ok to do so, or to just do a diff between the two patches.
If changes are needed after a reroll, a separate patch and interdiff are then made. I try not to mix a reroll with improvements.
Other new patches, I dont call rerolls, I just call them new patches. Those I almost always make interdiffs for.
=============
+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,232 @@
+ '#markup' => check_plain($source_array[0]),
#13 tried to do that but used String::check_plain instead of String::checkPlain
#21 put it back to the procedural
#23 pointed out that it should be checkPlain.
But here I do the use and String::checkPlain again. Let us see if this works this time.
looking at a previous time I did dependency injection: https://drupal.org/node/1945226#comment-7572819
... I'm going to post what I have so far and do the injection in a bit.
Comment #49
YesCT CreditAttribution: YesCT commentedcontinuing 8.
Looking at https://drupal.org/node/1953354 I was still confused, but tried anyway.
I blindly guess that the property might want to be localeStorage since it was
\Drupal::service('locale.storage')->whatever...
So we need a property so we can do
$this->localeStorage->whatever...
I found the type that that property should be (with help from @tim.plunkett and @xjm in irc)
by looking for a locale.services.yml file
and looking for locale.storage in there.
it says:
The type of the property though should be an interface whenever possible. "if there is no interface it's a bug"
So I opened up StringDatabaseStorage and it has:
class StringDatabaseStorage implements StringStorageInterface {
So, that gives me this part:
Looking at a previous example of injection, I see I will want a create() (or createInstance() if it is dealing with an EntityControllerInterface since that had a create() already) and __construct()
__construct() should set the property, so do: $this->localeStorage = $somethingPassedInAsAParameter and I know the type of the parameter should be the same type as the property.
So that gets me:
Since this is not an EntityController, I want a create() method.
Looking at:
abstract class TranslateFormBase implements FormInterface {
I check in FormInterface to see what it has for its create(), but there is no create.
Tim says FormInterface doesn't assume injection.
So use ControllerInterface because ... that's its purpose it only has create() and allows injection.
It has the pattern
public static function create(ContainerInterface $container);
That means I also want to implement that like:
abstract class TranslateFormBase implements ControllerInterface, FormInterface {
and then make a create like:
That new code means new use's also.
Attached is the interdiff for all that.
Comment #51
YesCT CreditAttribution: YesCT commentedoh...
I see that was talking about TranslateEditForm, and I did the injection in TranslationFormBase.
In TranslateEditForm it's already now
foreach ($this->localStorage->getTranslations(array('lid' => $lids, 'language' => $langcode, 'translated' => TRUE)) as $existing_translation_object) {
*which* makes me think it really should be localeStorage and not localStorage.
Note, @Crell thought in irc that we should use a better name that localeStorage because of the easy confusion with localStorage. Maybe stringStorage or something (see the question in #41.
Now I'm unsure if 49 is needed, but if it is, trying to make this work with that....
I think the TranslateEditForm create() is wrong now... not matching ControllerInterface
ah, I see looking at #39 I can get the state and translation from the container. Trying that.
Another thing to check is the Node $node = NULL argument in buildForm. It seems to be unused.
tim brought up the same point in #25
. I took those arguments out.
Also, translateFilterValues is expecting $request, but other places it is called are not passing the request in.
I'm not sure if that means request needs to be set somewhere in TranslateFilterForm...
We need to pass in $request to translateFilterValues, or handle the @todo.
If we take out the $request = \Drupal::request(); then we need to make sure to pass in request everywhere translateFilterValues is called.
here.
here also.
Do we put a $request property on TranslateFormBase?
we did change the constructor, so the first point in #25 is well taken. Making that change.
This needs addressed.
Next, I will take a look at the diffs in #43
Comment #52
YesCT CreditAttribution: YesCT commentedFrom diff-locale_translate_edit_form-TranslateEditForm-buildForm.txt
did we loose this Built-in English on purpose? I dont think so.
Looks like it was put in in #2004684: Improve Accessibility for String Translation UI on June 20.
--------
from diff-locale_translate_filter_form-TranslateFilterForm-buildForm.txt
June 7, #1987066: Rename files to match CSS file naming convention
---------
the rest of the diffs look like they have changes we intended.
I'm taking a break from this for a bit. It is available for anyone to take on.
Comment #54
victor-shelepen CreditAttribution: victor-shelepen commentedShoud be changed to :
TranslateFormBase already containes FormInterface.
Comment #55
victor-shelepen CreditAttribution: victor-shelepen commentedThe patch is workable. I push it to run tests.
Comment #56
victor-shelepen CreditAttribution: victor-shelepen commentedComment #57
victor-shelepen CreditAttribution: victor-shelepen commentedComment #59
victor-shelepen CreditAttribution: victor-shelepen commentedThe form TranslateEditForm has been corrected to pass test successfully. It was a bug that could be defined by manual testing. :)
Comment #61
victor-shelepen CreditAttribution: victor-shelepen commentedComment #62
victor-shelepen CreditAttribution: victor-shelepen commentedComment #63
dawehnerIn order to properly document which classes are containeraware this controller should implement the ContainerAwareInterface
We could also call parent::__construct to store the variables.
This code was just moved, so that is fine, but isn't there a better way then dealing with t() directly? Afaik there is something like triggered_element
It feels wrong to still have drupal_static here. Maybe we should replace it by a property on the object.
Comment #64
victor-shelepen CreditAttribution: victor-shelepen commentedComment #65
YesCT CreditAttribution: YesCT commentedIn case it helps, the second thing about the parent is like we did in #2045043-17: Field listings operations cannot be altered
Comment #66
victor-shelepen CreditAttribution: victor-shelepen commentedThe patch for the comment 61 has been corrected according to the comment 63.
I can not understand clear
Comment #67
Crell CreditAttribution: Crell commentedThe full container should never be passed to a ControllerInterface object. If you need a service, extract it in the create() method and inject it that way.
Forms should never be ContainerAware. Inject the services you need explicitly.
This is a bug. create() should return the form OBJECT. Not the result of drupal_get_form(). Any create() method that does more than return new static(...) is probably a bug.
1) You almost never want to use self:: in PHP 5.3. Use static:: instead.
2) That said, this line is totally screwy. An object should (almost) never write to the class, as that becomes a quasi-global and pollutes other instances. If the idea here is caching computed values, save that to a $this-> property, not to self::.
3) That said, don't bother caching a generated value unless you know it's actually needed. Excessive caching is just as much a problem as insufficient caching, but harder to fix later.
The translation service should be injected.
Ick, why are we *adding* a module_load_include()? We should be eliminating those.
Comment #68
victor-shelepen CreditAttribution: victor-shelepen commented@Crell. I agree with you partly. I also agree with Drupal way partly. I see that I should follow it. Your comments are different form the flow of this issues. I need somebody to review an issue and @Crell comments that are related with containers.
Comment #69
victor-shelepen CreditAttribution: victor-shelepen commentedFor @Crell #64
Why not? I need it inject deeper into another objects like in the
Comment #70
victor-shelepen CreditAttribution: victor-shelepen commentedFor @Crell #64 1.
Why not? ;) We have added some rules - Interface and helpful methods from. It works fine. Interface helps people see why we need it.
Comment #71
victor-shelepen CreditAttribution: victor-shelepen commentedFor @Crell #64
Maybe. LSB it is the very interesting topic. The feature exists in the scope. Here data should be shared between classes those inherit this class TranslateFormBase. So data is inherited between classes: TranslateFilterForm, TranslateEditForm. drupal_static has been used earlier. But it does not support methods of classes well. So if we use static, data will be saved in the current scopes. So same operations will be processed twice. So it is equal if we do not use this field in the static way. It is almost the same situation when data is saved into a field of a class instance.
It likes a hack way now. I think we need use a common context.
I agree with another comments.
Comment #72
tim.plunkettYou should still use constructor injection to properly declare your other dependencies, and only use $this->container for those two forms.
And please make the switch from self:: to static::, it's an existing pattern that we've tried to adhere to 100%.
Comment #73
victor-shelepen CreditAttribution: victor-shelepen commentedI see it needs to be converted from ContainerInterface to ContainerAware.
Comment #74
tim.plunkettNo, from ContainerAware to ControllerInterface
Comment #75
victor-shelepen CreditAttribution: victor-shelepen commentedDone:
Also:
Comment #77
victor-shelepen CreditAttribution: victor-shelepen commentedThe test has been fixed.
Comment #78
victor-shelepen CreditAttribution: victor-shelepen commentedComment #80
victor-shelepen CreditAttribution: victor-shelepen commentedComment #81
victor-shelepen CreditAttribution: victor-shelepen commentedComment #82
Gábor HojtsyAs per #2057155: Add a generateFromRoute() method on the url generator to accept options like url(), this should now be Drupal\Core\Routing\UrlGeneratorInterface, no?
I don't think we should have this @see, that hook will go away...
Several extra line-end whitespaces.
I've seen elsewhere $this->t() is already on containers/forms? Is that the case? Would be much better.
Some standalone t()s left around. I honestly don't think leaving the others standalone t() would be fine for now IMHO, we don't need to do all the conversions for all the best practices as once... but if we do conversion for something at least do it consistently.
Extra whitespace.
Comment #83
Gábor Hojtsy#80: locale-convert_locale_translate_page-1978916-77.patch queued for re-testing.
Comment #85
Gábor HojtsyIndeed as per #2018411-34: Figure out a nice DX when working with injected translation using $this->t() is the only good way. If the base class for whatever you are using here does not include that method, just go back to t() the function call and ignore trying to do these replacements. We gotta get this in sooner than later :)
Comment #86
tim.plunkettAny controller or form can has $this->t() available to it.
Comment #87
disasm CreditAttribution: disasm commentedreroll
Comment #88
disasm CreditAttribution: disasm commentedMassive clean-up:
Extended FormBase
Used $this->container in constructs so don't have to replicate create() everywhere
Did regexp search replace t() -> $this->t()
Dig regexp search replace $this->translationManager->translate -> $this->t()
Even though there are a decent number of function calls in these methods, since we're focusing on getting rid of a new router, creating a translation service can be done in another issue IMHO.
Comment #89
tim.plunkettFormBase is different than ControllerBase, it is not ContainerAware. You'll still need
public static function create()
It's actually considered bad practice to store
$this->request
. Just use $this->getRequest() wherever its needed. (You can't trust that getRequest() or setRequest() has been called yet)$reset = FALSE
Comment #91
disasm CreditAttribution: disasm commentedattached addresses comments in #90.
Comment #93
tim.plunkettWe should not store the container if at all possible. Also how is it even been passed to the constructor?
This should be handled in create()
{@inheritdoc}
A) This should be right below __construct()
B) We shouldn't pass container, get the services you need.
The request looks great.
Comment #94
disasm CreditAttribution: disasm commentedComment #96
disasm CreditAttribution: disasm commentedOne more try for the night. Completely removing constructor in Controller, and fixing errors in base form.
Comment #97
tim.plunkett3 times I think people have reviewed this and asked for the switch/case to be removed.
I added "@todo Fix this before commit." to make sure that happens.
Comment #98
tim.plunkettAlso this is weird.
Comment #100
disasm CreditAttribution: disasm commentedsplit the methods, however; not sure where it needs defined that the submit action for the reset button is different. Also, had some changes locally I was working on, and your interdiff didn't apply over them, so I just manually went through the diff and added the bits I didn't have yet.
Comment #102
tim.plunkettNot sure what this does, and its not used in resetForm.
is how you get the correct submit.
Comment #103
disasm CreditAttribution: disasm commentedlets try this one. Fixing submit handler and removing the extraneous line in resetForm.
Comment #105
tim.plunkettRedoing my work in #97.
Comment #106
Letharion CreditAttribution: Letharion commentedNo longer applied. Re-rolled on top of the ContainerInjectionInterface change.
Comment #107
jibran$this->t
Comment #108
dawehner.
Comment #109
Letharion CreditAttribution: Letharion commentedFixed #107.
Comment #110
Gábor HojtsyI did not review each line but spot-checking and looking for the things I complained about earlier, this looks good to go!
Comment #111
tim.plunkettPart of LocaleController::checkTranslation() has no test coverage. The last patch uses $this->redirect with a path, while that only works with route name.
Basically, admin/reports/translations/check is never visited when the "Translation source" (
translation.use_source
) is set to "Local files only".Please open a follow-up to test that.
In addition, several variables were only defined inside foreach loops, which can cause bugs in certain circumstances.
Finally, FormBase implements ContainerInjectionInterface already, and this patch tried to implement ControllerInterface.
Comment #112
Gábor Hojtsy@tim.plunkett: neither '/check', neither "Translation source", neither "use_source" is mentioned in this patch. Not sure what you observed and how is it related this issue?
Not sure what are the missing pieces, your comment is not clear, and the issue summary is only up to date up to #53 with remaining tasks...
Comment #113
Gábor HojtsyComment #114
CaDyMaN CreditAttribution: CaDyMaN commented#44: 1978916-locale-convert_locale_translate_page-44.patch queued for re-testing.
Comment #115
tim.plunkettThe patch in #109 has
And that should be an error. generateFromPath() returns a URL, redirect() expects a route name.
Therefore this conversion touches code that has no test coverage.
Comment #116
Gábor HojtsyAll right, are we supposed to write test coverage as well while doing conversions? Last time I heard there is a plan B that the procedural functions will just be wrapped in minimal OO wrappers as a fire-sure way to get the conversions "done" sooner, and then we can fiddle forward with all-ecompassing issues. I personally think it would be more respectful of people's time spent on these issues to let conversions that look fine get in vs. holding it up on more perfection, like writing even more test coverage for stuff. Otherwise looks like the other option is this will go on for a while, then get ignored for the fast track crappy solution and we end up with lots of contributor time wasted and a much worse solution at the end. I'm a big fan of moving ahead with smaller steps and not trying to cover everything at once. It is not just more sensible for people so they have some success (this issue has been worked on for almost 5 months) but also more sensible for core progress.
Comment #117
tim.plunkettThat's why I tagged it "Needs followup", not "Needs tests". I'm not a monster ;)
Comment #118
Gábor HojtsyRight, I asked about tasks remaining. So looks like none for this issue.
Comment #119
Gábor HojtsyOpened followup at #2085065: Test coverage missing for translation check when local files only is configured
Comment #120
Gábor HojtsyComment #120.0
Gábor Hojtsyadded remaining tasks.
Comment #121
webchickAwesome work on this, folks!
Committed and pushed to 8.x. Thanks!
Comment #122
Gábor HojtsyWoot, thanks!
Comment #123
tstoecklerI went over this patch but could not find any place ContentLanguageSettingsForm is actually used. I didn't see any discussion on that here, either. Can anyone comment on that?
Comment #124
tim.plunkettUm, yeah, that's weird. I didn't even notice when I was reviewing, that's embarrassing.
Comment #126
penyaskito#124: language-1978916-124.patch queued for re-testing.
Comment #128
penyaskitoStraight reroll.
Interdiff is between patch files with diff -u.
Comment #130
jibranWe have to remove this function as well and move it to controller.
Comment #131
googletorp CreditAttribution: googletorp commentedI did some work on this patch cleaning it up some more, changing the router and the test case to use the new configuration based form.
I ran into a problem I couldn't quite solve. It seems somethings language_configuration_element_submit doesn't get $form passed by refference in the batch job. Changing the function signature or what _batch_set_next does seems breaks the config form.
I'm hoping some one can fix the notices.
Setting to needs review to have the tests run.
Comment #133
victor-shelepen CreditAttribution: victor-shelepen commentedThe mess is going from this place https://drupal.org/node/1978916#comment-7865513. It overlaps with another issue https://drupal.org/node/1987726.
Comment #134
victor-shelepen CreditAttribution: victor-shelepen commentedWe do not need touch language_content_settings_page here.
Comment #135
disasm CreditAttribution: disasm commentedClosing this. This patch was already committed, and all the ContentLanguageSettingsForm stuff is being done in #1987726: Convert language content page related callback to new style controller.
Comment #135.0
disasm CreditAttribution: disasm commentedUpdate remaining tasks
Comment #136
cilefen CreditAttribution: cilefen commented#2551259: Deprecate dead code locale_translation_manual_status()