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.

This issue covers below changes:

  1. removed helper language_entity_supported
  2. Convert content_language_settings_page to new controller
  3. Convert content_language_settings_form to new formController

Blocked/Postponed on this:

#933004: Test that all form elements have a title for accessibility

Follow-up (postponed on this):

#2241727: Remove button name override and update tests for that rename of Save to Save configuration in

Being done here, issues marked duplicate of this one:

#2138151: ContentLanguageSettingsForm is unused (and broken)

Might need a re-roll when this goes in

#2115427: Convert language_content_settings_form to FormInterface

Files: 
CommentFileSizeAuthor
#127 1987726-diff-125-127.txt803 bytesvijaycs85
#127 1987726-language-controller-127.patch11.1 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,105 pass(es).
[ View ]
#125 language-content_settings_form-1987726-119.patch11.12 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,049 pass(es).
[ View ]
#122 whatthediff.png89.69 KBYesCT
#121 language-content_settings_form-1987726-121.patch10.3 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,914 pass(es), 38 fail(s), and 2,567 exception(s).
[ View ]
#121 interdiff-1987726-119-121.txt1.62 KBYesCT
#119 language-content_settings_form-1987726-119.patch11.12 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,829 pass(es).
[ View ]
#119 interdiff-1987726-116-119.txt775 bytesYesCT
#116 language-content_settings_form-1987726-116.patch11.08 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,789 pass(es).
[ View ]
#109 language-content_settings_form-1987726-107-109.txt2.43 KBlikin
#109 language-content_settings_form-1987726-109.patch11.06 KBlikin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,874 pass(es).
[ View ]
#107 drupal_1987726_107.patch11.22 KBXano
FAILED: [[SimpleTest]]: [MySQL] 64,491 pass(es), 75 fail(s), and 1 exception(s).
[ View ]

Comments

Assigned:Unassigned» pdrake

Status:Active» Needs review
StatusFileSize
new10.32 KB
FAILED: [[SimpleTest]]: [MySQL] 56,945 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new11.38 KB
PASSED: [[SimpleTest]]: [MySQL] 56,637 pass(es).
[ View ]

Manual testing the patch works pretty good.

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.phpundefined
@@ -0,0 +1,132 @@
+  /**
+   * Return a list of entity types for which language settings are supported.
+   */
+  protected function entitySupported() {
+    foreach (entity_get_info() as $entity_type => $info) {
+      if (!empty($info['translatable'])) {
+        $supported[$entity_type] = $entity_type;
+      }
+    }
+    return $supported;

It's odd to actually move an api function into a protected method on the controller, even there is just a single user of it.

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.phpundefined
@@ -0,0 +1,132 @@
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

This could use {@inheritdoc}

I moved the API function because this is the only use in core and I could not locate any uses in contrib for it. I could certainly make it a public function or simply put it back where it was, but it seemed to only serve a function within the content language settings form.

Well, it feels really helpful. Thanks for searching in contrib, but afaik the language module got introduced in D8 so there is a low chance that actually these functions are used already.

I asked the i18n people for their opinion.

StatusFileSize
new11.26 KB
PASSED: [[SimpleTest]]: [MySQL] 56,946 pass(es).
[ View ]

@dawehner, thanks for inquiring with i18n folks. Are there any outstanding issues with the patch besides making a final determination on that function?

As far as I understood gabor he said that people which deal with kind of problems need other information from the entity info anyway so they don't really loose anything by not heaving this function.

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.phpundefined
@@ -0,0 +1,132 @@
+   * Return a list of entity types for which language settings are supported.

Needs an @return

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.phpundefined
@@ -0,0 +1,132 @@
+    $form = array(
+      '#labels' => $labels,
+      '#attached' => array(
+        'css' => array($path . '/language.admin.css'),
+      ),

This should be better an entry in hook_library.

StatusFileSize
new2.42 KB
new12.99 KB
FAILED: [[SimpleTest]]: [MySQL] 55,841 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new7.78 KB
FAILED: [[SimpleTest]]: [MySQL] 56,874 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1987726-convert_language_content_settings_to_controller-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
new8.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987726-convert_language_content_settings_to_controller-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixing test case...

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1987726-convert_language_content_settings_to_controller-15.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1987726-convert_language_content_settings_to_controller-15.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion

The last submitted patch, 1987726-convert_language_content_settings_to_controller-15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.07 KB
new8.78 KB
FAILED: [[SimpleTest]]: [MySQL] 59,992 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

Patch reroll

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1987726-convert_language_content_settings_to_controller-21.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion

The last submitted patch, 1987726-convert_language_content_settings_to_controller-21.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.57 KB
new14.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1987726-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

That is just fixing some other unrelated issues.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, drupal-1987726-25.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion

#25: drupal-1987726-25.patch queued for re-testing.

Issue tags:+D8MI

Status:Needs review» Needs work

The last submitted patch, drupal-1987726-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.4 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/language/language.module.
[ View ]

Status:Needs review» Needs work

The last submitted patch, language_manager-1987726-30.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+LONDON_2013_JULY
StatusFileSize
new6.36 KB
FAILED: [[SimpleTest]]: [MySQL] 57,504 pass(es), 32 fail(s), and 0 exception(s).
[ View ]

Re-roll with syntax error fix.

StatusFileSize
new5.77 KB
new12.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,569 pass(es), 4 fail(s), and 36 exception(s).
[ View ]

Adding the file that is missing in #32.

Status:Needs review» Needs work

The last submitted patch, 1987726-convert_language_content_settings_controller-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.17 KB
FAILED: [[SimpleTest]]: [MySQL] 58,424 pass(es), 2 fail(s), and 40 exception(s).
[ View ]

reroll

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.1987726-35.patch, failed testing.

Thanks 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.

Issue tags:+sprint, +language-content

In short, we have 3 more days to get this to RTBC or we'll need to split this to two issues and do it in 2 steps. Anybody wanna jump on this? :)

Status:Needs work» Needs review
StatusFileSize
new4.74 KB
new10.69 KB
FAILED: [[SimpleTest]]: [MySQL] 59,176 pass(es), 4 fail(s), and 25 exception(s).
[ View ]

Interestingly, this form is already in core, just not being used, and it's slightly broken. Fixing the form, removing the the form callbacks. I'm not sure splitting this one is going to help any. All the test failures are a result of hook_element_info not processing things correct. The code in the form is nearly identical (with a performance improvement, but I verified same behavior without it).
Here's the errors for anyone that wants to take a look:

Notice: Undefined index: settings[comment][comment_node_article][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[comment][comment_node_page][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[node][article][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[node][page][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[custom_block][basic][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[menu_link][account][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[menu_link][admin][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[menu_link][footer][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[menu_link][main][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[menu_link][tools][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[menu_link][shortcut][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[taxonomy_term][tags][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).
Notice: Undefined index: settings[user][user][settings][language] in language_configuration_element_submit() (line 318 of core/modules/language/language.module).

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.1987726-39.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new614 bytes
new10.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.language-module.1987726-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

still haven't fixed index error. This resolves the VERSION error though by removing the unused library (single css makes more sense as a css attached anyways).

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.1987726-41.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.1 KB
new10.97 KB
FAILED: [[SimpleTest]]: [MySQL] 59,139 pass(es), 2 fail(s), and 18 exception(s).
[ View ]

Reroll. Using drupalPostForm instead of drupalPost.

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.1987726-43.patch, failed testing.

Hah, how is the ViewAjaxControllerTest.php change related?

Save -> Save Configuration: The SettingsFormBase uses Save Configuration as the text on the Submit button.

Status:Needs work» Needs review
StatusFileSize
new10.18 KB
PASSED: [[SimpleTest]]: [MySQL] 59,306 pass(es).
[ View ]
new6.23 KB

Let's try this.
I think no reason to change button text in conversion.

Assigned:pdrake» Unassigned

Un-assigning this issue. I will review it.

StatusFileSize
new9.72 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

[Dupe]

StatusFileSize
new9.72 KB
FAILED: [[SimpleTest]]: [MySQL] 58,725 pass(es), 28 fail(s), and 14 exception(s).
[ View ]

I had re-roll #47 patch because there was three conflicts in the head. First conflict was in language.module where router had different name 'route_name' => 'language.content_settings_page' so I left this route name. Second conflict was in language.routing.yml had title under default, and third conflict was in language.admin.inc which had just this one line (@deprecated Use \Drupal\language\Controller\LanguageController::contentSettings()) added for @deprecated Use \Drupal\language\Controller\LanguageController::contentSettings().

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.1987726-49.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.97 KB
FAILED: [[SimpleTest]]: [MySQL] 58,521 pass(es), 28 fail(s), and 14 exception(s).
[ View ]

Status:Needs review» Needs work
Issue tags:-D8MI, -sprint, -language-content, -WSCCI-conversion, -LONDON_2013_JULY

The last submitted patch, drupal8.language-module-1987726-53.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+D8MI, +sprint, +language-content, +WSCCI-conversion, +LONDON_2013_JULY

The last submitted patch, drupal8.language-module-1987726-53.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
new20.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,251 pass(es).
[ View ]

Fixes test failures. The temporary controller and route info needed removed.

Status:Needs review» Reviewed & tested by the community

+++ w/core/modules/content_translation/content_translation.admin.inc
@@ -514,8 +509,13 @@ function content_translation_translatable_batch($translatable, $field_name, &$co
+ *
+ * @param \Drupal\Core\Entity\EntityInterface $entity
+ *   The entity to update.
+ * @param string $field_name
+ *   The field to update.
  */
-function _content_translation_update_field($entity_type, EntityInterface $entity, $field_name) {
+function _content_translation_update_field(EntityInterface $entity, $field_name) {
   $empty = 0;

Nice, we actually add documentation.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new20.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,824 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Either we have scope creep here or the issue summary needs work because this patch is definitely not just doing what it says "Convert language_content_settings_page() to a new style controller"

Can we either have an issue summary and title update along with some explanation or we can separate out some of the changes introduced by #53.

Seems better to postpone the issue on the same reason as #1946462-59: Convert content_translation_translatable_form() to the new form interface

Title:Convert language_content_settings_page() to a new style controllerConvert language content page related callback to new style controller
Issue tags:+London_2013_October

Seems we need to combine them to make work. Updating title and issue summary with scope of the patch.

Status:Needs work» Needs review
Issue tags:-D8MI, -sprint, -language-content, -WSCCI-conversion, -LONDON_2013_JULY, -London_2013_October

#61: drupal_1987726_61.patch queued for re-testing.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

Patch fits the current scope. :)

Restoring tags and +1 for RTBC.

61: drupal_1987726_61.patch queued for re-testing.

61: drupal_1987726_61.patch queued for re-testing.

This seems to combine #2115427: Convert language_content_settings_form to FormInterface with something else, which is not clear. That issue has a passing patch, could we just finish that and then rescope this appropriately?

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
@@ -157,7 +144,7 @@ public function buildForm(array $form, array &$form_state) {
-    return parent::buildForm($form, $form_state);
+    return $form;

This change means that $form['#theme'] = 'system_config_form' will no longer be applied. I think it makes more sense to remove the declaration of the save button above and retain the call to parent::buildForm(). The only difference is the label of the button ('Save' vs. 'Save configuration'). Above (around #47 it was decided that we want to leave the button label for now, in order to not break tests (at least I think that was the reason). In that case we should call parent::buildForm() and then override the button label.

Also #2138151: ContentLanguageSettingsForm is unused (and broken) was marked as a duplicate of this, for reference.

Just want to add a note to get back to this #933004: Test that all form elements have a title for accessibility which is postponed based on this issue.

Status:Needs work» Needs review
StatusFileSize
new927 bytes
new20.77 KB
FAILED: [[SimpleTest]]: [MySQL] 59,145 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here's a re-roll for #72. Also noted that #61 was missing #button_type =>primary. So this was not only a Themer's experience issue, but also a UX issue.

Status:Needs review» Needs work

The last submitted patch, 75: 1987726-75.patch, failed testing.

Status:Needs work» Needs review

75: 1987726-75.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 75: 1987726-75.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Random fail

Been seeing this a lot lately.

ImageFieldDisplayTest.php, line 229
ImageFieldDisplayTest.php, line 251

array_flip(): Can only flip STRING and INTEGER values!
in FieldableDatabaseStorageController.php, line 212

75: 1987726-75.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 75: 1987726-75.patch, failed testing.

Status:Needs work» Needs review

75: 1987726-75.patch queued for re-testing.

Why do you hate me so much, testbot? I've always tried to be nice to you... :'-(

Status:Needs review» Needs work

The last submitted patch, 75: 1987726-75.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.89 KB
FAILED: [[SimpleTest]]: [MySQL] 59,098 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 85: 1987726-language-controller-85.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.89 KB
FAILED: [[SimpleTest]]: [MySQL] 59,476 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

The patches failed on different tests, so just re-uploading the latest patch.

Is there a reason why the patch size dropped by 50% from #75 to #85?

The changes to content_translation_translatable_batch are gone. But that seems fine, since they weren't really in scope.

content_translation_translatable_batch was removed in #2076445: Make sure language codes for original field values always match entity language regardless of field translatability anyway, so no need to cover it anymore :)

@dawehner, @tim.plunkett: otherwise looks good? :)

Status:Needs review» Needs work

The last submitted patch, 87: 1987726-language-controller-85.patch, failed testing.

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
@@ -53,22 +53,6 @@ public static function create(ContainerInterface $container) {
-   * Return a list of entity types for which language settings are supported.
-   *
-   * @return array
-   *   A list of entity types which are translatable.
-   */
-  protected function entitySupported() {
-    $supported = array();
-    foreach ($this->entityManager->getDefinitions() as $entity_type => $info) {
-      if (!empty($info['translatable'])) {
-        $supported[$entity_type] = $entity_type;
-      }
-    }
-    return $supported;
-  }
-
-  /**
    * {@inheritdoc}

Should we not have some kind of service to provide this information for other people?

contentTranslationManager::getSupportedEntityTypes() is the service method to get that from :) The logic there is !empty($info['translatable']) && !empty($info['links']['drupal:content-translation-overview']) so slightly more strict but basically the same.

StatusFileSize
new10.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,839 pass(es).
[ View ]

I don't buy it, Mr. Bot. Re-re-uploading.

+++ b/core/modules/language/language.module
@@ -125,22 +125,6 @@ function language_theme() {
-function language_entity_supported() {

This looks like needs API change tag

Status:Needs work» Needs review

Lets test the last patch...

StatusFileSize
new10.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987726-language-controller-100_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch has been rerolled successfully..

Status:Needs review» Needs work

The last submitted patch, 101: 1987726-language-controller-100_1.patch, failed testing.

The last submitted patch, 101: 1987726-language-controller-100_1.patch, failed testing.

Issue tags:-sprint

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 101: 1987726-language-controller-100_1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.22 KB
FAILED: [[SimpleTest]]: [MySQL] 64,491 pass(es), 75 fail(s), and 1 exception(s).
[ View ]

Re-roll.

Status:Needs review» Needs work

The last submitted patch, 107: drupal_1987726_107.patch, failed testing.

StatusFileSize
new11.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,874 pass(es).
[ View ]
new2.43 KB

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 109: language-content_settings_form-1987726-109.patch, failed testing.

Status:Needs work» Needs review

Ok, it is green and needs review.

Issue tags:-Random fail+Random test failure

Status:Needs review» Needs work
Issue tags:+Needs reroll

I'm reviewing this. But it does not apply. So I'm also re-rolling it now. https://drupal.org/patch/reroll

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new11.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,789 pass(es).
[ View ]

patch was removing language_content_settings_page()
and in head there was a change to that function.

patch was expecting to remove line:
return drupal_get_form('language_content_settings_form', language_entity_supported());

in head is now:
return \Drupal::formBuilder()->getForm('language_content_settings_form', language_entity_supported());

because of
#2121175: Remove usage of drupal_get_form()

--
just removing the hunk as in the patch.
(no interdiff since it was a reroll, but that was the difference)
--
attaching patch to see what testbot says and I'll continue to review.

ok, I was wondering if we should keep entitySupported as something like getTranslatableEntities ...

[edit: this was brought up in #94 and #95]

I looked at its one use (which was in the form) and I see we already did the getDefinitions at the beginning, and that entitySupported was doing that again. so, deleting this is ok.

*if* we kept a getTranslatableEntities (aka entitySupported) I guess we would have wanted to do

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
@@ -75,14 +75,14 @@ public function getFormId() {
    * {@inheritdoc}
    */
   public function buildForm(array $form, array &$form_state) {
-    $entity_types = $this->entityManager->getDefinitions();
+    $translatable_entity_types = $this->entitySupported();
     $labels = array();
     $default = array();
     $bundles = entity_get_bundles();
     $language_configuration = array();
-    foreach ($this->entitySupported() as $entity_type_id) {
-      $labels[$entity_type_id] = $entity_types[$entity_type_id]->getLabel() ?: $entity_type_id;
+    foreach ($translatable_entity_types as $entity_type_id) {
+      $labels[$entity_type_id] = $translatable_entity_types[$entity_type_id]->getLabel() ?: $entity_type_id;
       $default[$entity_type_id] = FALSE;
       // Check whether we have any custom setting.

Nothing needs work so far. Just taking notes.

read all the comments and summarize the inter-relations of issues relevant here.

StatusFileSize
new775 bytes
new11.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,829 pass(es).
[ View ]
  1. +++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
    @@ -79,14 +63,17 @@ public function buildForm(array $form, array &$form_state) {
    -    $bundles = entity_get_bundles();
    +    $bundles = $this->entityManager->getAllBundleInfo();
    ...
    -      foreach ($bundles as $bundle => $bundle_info) {
    +      foreach ($bundles[$entity_type_id] as $bundle => $bundle_info) {

    I think we might be able to just get the bundle info for the entities we are interested in the foreach bundle loop below. But that should not be part of this conversion.

    Maybe would make a good separate novice issue.

    #2241661: Get bundle info for just the entities we need instead of calling getAllBundleInfo() in ContentLanguageSettingsForm

    Oh, nevermind. wont fixed that since getBundleInfo calls getAllBundleInfo anyway.

  2. +++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
    @@ -147,13 +134,12 @@ public function buildForm(array $form, array &$form_state) {
    +    $form = parent::buildForm($form, $form_state);
    +    // @todo Remove this override. There are tests that check for explicitly for
    +    //   the button label which need to be adapted for that.
    +    $form['actions']['submit']['#value'] = $this->t('Save');

    in #75 @tstoeckler added this @todo. Since this @todo is mean to be done after this issue, and not before this issue is rtbc, we need a follow-up issue for it, and to link to it.

    Doing that.

    And it is listed in the issue summary.

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
@@ -163,16 +150,14 @@ public function submitForm(array &$form, array &$form_state) {
-    parent::submitForm($form, $form_state);
+    drupal_set_message($this->t('Settings successfully updated.'));

this looked a little strange, but I see in #1946404: Convert forms in field_ui.admin.inc to the new form interface it did something similar. In FieldInstanceEditForm in submitForm() it calls ->save and then drupal_set_message().

StatusFileSize
new1.62 KB
new10.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,914 pass(es), 38 fail(s), and 2,567 exception(s).
[ View ]

1.

+++ b/core/modules/language/language.admin.inc
@@ -152,105 +152,6 @@ function theme_language_negotiation_configure_browser_form_table($variables) {
-function language_content_settings_form(array $form, array $form_state, array $supported) {

this was already moved (copied) to buildForm() in #1978916: Convert locale_translate_page to a Controller which had the patch in #111 committed in comment #121

(nothing todo here, just trying to figure out why that code didn't move within this issue)

2.

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
@@ -79,14 +63,17 @@ public function buildForm(array $form, array &$form_state) {
-    $bundles = entity_get_bundles();
+    $bundles = $this->entityManager->getAllBundleInfo();
...
-      foreach ($bundles as $bundle => $bundle_info) {
+      foreach ($bundles[$entity_type_id] as $bundle => $bundle_info) {
@@ -131,7 +118,7 @@ public function buildForm(array $form, array &$form_state) {
-      foreach ($bundles as $bundle => $bundle_info) {
+      foreach ($bundles[$entity_type_id] as $bundle => $bundle_info) {

I was wondering if this would work without this change. I searched the old patches and see this change was introduced by @disasm in #39.

since I am curious, this patch is just to find out.

StatusFileSize
new89.69 KB

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
@@ -163,16 +150,14 @@ public function submitForm(array &$form, array &$form_state) {
-          $config->set(language_get_default_configuration_settings_key($entity_type, $bundle),
-            array(
-              'langcode' => $bundle_settings['settings']['language']['langcode'],
-              'language_show' => $bundle_settings['settings']['langcode']['language_show'],
-            )
-          );
+        $config->set(language_get_default_configuration_settings_key($entity_type, $bundle), array(
+          'langcode' => $bundle_settings['settings']['language']['langcode'],
+          'language_show' => $bundle_settings['settings']['language']['language_show'],
+        ));

this change is completely unrelated.

it's a whitespace fix.
if we want to do that as a novice follow-up I'm ok with that.

but I'm taking it out of here.

doh! I undid this and did a git diff --color-words just to make sure, and look:

there was a change.

done in #39 by @disasm
to actually fix tests. so leaving this.

I was doing manual testing but need to go do something else for a few hours.

Status:Needs review» Needs work

The last submitted patch, 121: language-content_settings_form-1987726-121.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new11.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,049 pass(es).
[ View ]

note this is the *same* patch as #119. (121 was an experiment to see if it would fail and it did. so 119 is what we want).

manual testing shows the form still works (there are some unrelated things broken about content translation, but nothing I saw related to this issue).

only code change I did while doing this review was adding the issue link for the @todo, so this is rtbc as far as i can tell.

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/language/language.routing.yml
    @@ -78,6 +78,6 @@ language.content_settings_page:
    -    _content: '\Drupal\language\Controller\LanguageController::contentSettings'
    +    _form: 'Drupal\language\Form\ContentLanguageSettingsForm'

    So we've just had the form converted but not being used? Funny.

  2. +++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
    @@ -79,14 +63,17 @@ public function buildForm(array $form, array &$form_state) {
    +      $labels[$entity_type_id] = $entity_type->getLabel() ? $entity_type->getLabel() : $entity_type;

    This looks wrong since $entity_type is the object - this change looks unnecessary. In fact there is code that uses $labels that assumes this is a string so this would actually break stuff if we ever add a entity type with out a label.

Status:Needs work» Needs review
StatusFileSize
new11.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,105 pass(es).
[ View ]
new803 bytes

#126.1 - Yes, it looks like and we are removing this temp callback as part of this issue.
#126.2 - Not sure, how & when but this line has changed somehow from existing code:

<?php
// in language_content_settings_form
 
foreach ($supported as $entity_type_id) {
   
$labels[$entity_type_id] = $entity_types[$entity_type_id]->getLabel() ?: $entity_type_id;
?>

Updated now.