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

CommentFileSizeAuthor
#133 1987726-language-controller-133.patch11.08 KBGábor Hojtsy
#127 1987726-diff-125-127.txt803 bytesvijaycs85
#127 1987726-language-controller-127.patch11.1 KBvijaycs85
#125 language-content_settings_form-1987726-119.patch11.12 KBYesCT
#122 whatthediff.png89.69 KBYesCT
#121 language-content_settings_form-1987726-121.patch10.3 KBYesCT
#121 interdiff-1987726-119-121.txt1.62 KBYesCT
#119 language-content_settings_form-1987726-119.patch11.12 KBYesCT
#119 interdiff-1987726-116-119.txt775 bytesYesCT
#116 language-content_settings_form-1987726-116.patch11.08 KBYesCT
#109 language-content_settings_form-1987726-107-109.txt2.43 KBvictor-shelepen
#109 language-content_settings_form-1987726-109.patch11.06 KBvictor-shelepen
#107 drupal_1987726_107.patch11.22 KBXano
#101 1987726-language-controller-100_1.patch10.82 KBprateek479
#96 1987726-language-controller-85.patch10.89 KBtstoeckler
#87 1987726-language-controller-85.patch10.89 KBtstoeckler
#85 1987726-language-controller-85.patch10.89 KBvijaycs85
#75 1987726-75.patch20.77 KBtstoeckler
#75 1987726-61-75-interdiff.txt927 byteststoeckler
#61 drupal_1987726_61.patch20.54 KBXano
#57 drupal8.language-module.1987726-57.patch20.38 KBdisasm
#57 interdiff.txt1.31 KBdisasm
#53 drupal8.language-module-1987726-53.patch20.97 KBkgoel
#49 drupal8.language-module.1987726-49.patch9.72 KBkgoel
#50 drupal8.language-module.1987726-49.patch9.72 KBkgoel
#47 interdiff.txt6.23 KBandypost
#47 1987726-lang-settings-47.patch10.18 KBandypost
#43 drupal8.language-module.1987726-43.patch10.97 KBdisasm
#43 interdiff.txt1.1 KBdisasm
#41 drupal8.language-module.1987726-41.patch10.96 KBdisasm
#41 interdiff.txt614 bytesdisasm
#39 drupal8.language-module.1987726-39.patch10.69 KBdisasm
#39 interdiff.txt4.74 KBdisasm
#35 drupal8.language-module.1987726-35.patch12.17 KBdisasm
#33 1987726-convert_language_content_settings_controller-33.patch12.13 KBvijaycs85
#33 1987726-diff-32-33.txt5.77 KBvijaycs85
#32 1987726-convert_language_content_settings_controller-32.patch6.36 KBvijaycs85
#30 language_manager-1987726-30.patch12.4 KBdawehner
#25 drupal-1987726-25.patch14.21 KBdawehner
#25 interdiff.txt7.57 KBdawehner
#21 1987726-convert_language_content_settings_to_controller-21.patch8.78 KBstella
#21 1987726-15-21-interdiff.txt4.07 KBstella
#15 1987726-convert_language_content_settings_to_controller-15.patch8.99 KBvijaycs85
#15 1987726-diff-13-15.txt1.21 KBvijaycs85
#13 1987726-convert_language_content_settings_to_controller-13.patch7.78 KBvijaycs85
#11 drupal-convert_language_content_settings_to_controller-1987726-11.patch12.99 KBpdrake
#11 interdiff.txt2.42 KBpdrake
#8 drupal-convert_language_content_settings_to_controller-1987726-7.patch11.26 KBpdrake
#4 drupal-convert_language_content_settings_to_controller-1987726-4.patch11.38 KBpdrake
#2 drupal-convert_content_settings_page_callback_to_controller-1987726-2.patch10.32 KBpdrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pdrake’s picture

Assigned: Unassigned » pdrake
pdrake’s picture

Status: Active » Needs review
FileSize
10.32 KB

Status: Needs review » Needs work
pdrake’s picture

Status: Needs work » Needs review
FileSize
11.38 KB
dawehner’s picture

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}

pdrake’s picture

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.

dawehner’s picture

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.

pdrake’s picture

pdrake’s picture

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

dawehner’s picture

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.

pdrake’s picture

Status: Needs review » Needs work
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
7.78 KB

Status: Needs review » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
8.99 KB

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.

vijaycs85’s picture

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.

stella’s picture

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.

stella’s picture

Status: Needs work » Needs review
FileSize
4.07 KB
8.78 KB

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.

stella’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.57 KB
14.21 KB

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.

vijaycs85’s picture

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

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

vijaycs85’s picture

Issue tags: +D8MI

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.4 KB

Status: Needs review » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +LONDON_2013_JULY
FileSize
6.36 KB

Re-roll with syntax error fix.

vijaycs85’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
12.17 KB

reroll

Status: Needs review » Needs work

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

xjm’s picture

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.

Gábor Hojtsy’s picture

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? :)

disasm’s picture

Status: Needs work » Needs review
FileSize
4.74 KB
10.69 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
614 bytes
10.96 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
10.97 KB

Reroll. Using drupalPostForm instead of drupalPost.

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

Hah, how is the ViewAjaxControllerTest.php change related?

disasm’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
10.18 KB
6.23 KB

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

kgoel’s picture

Assigned: pdrake » Unassigned

Un-assigning this issue. I will review it.

kgoel’s picture

kgoel’s picture

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().

andypost’s picture

Status: Needs review » Needs work

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

kgoel’s picture

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.

googletorp’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
20.38 KB

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

dawehner’s picture

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.

Xano’s picture

alexpott’s picture

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

Patch no longer applies.

Xano’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
20.54 KB
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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.

andypost’s picture

vijaycs85’s picture

Title: Convert language_content_settings_page() to a new style controller » Convert 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.

vijaycs85’s picture

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.

vijaycs85’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

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

Patch fits the current scope. :)

vijaycs85’s picture

Restoring tags and +1 for RTBC.

Xano’s picture

61: drupal_1987726_61.patch queued for re-testing.

Xano’s picture

61: drupal_1987726_61.patch queued for re-testing.

tim.plunkett’s picture

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?

tstoeckler’s picture

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.

tstoeckler’s picture

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

mgifford’s picture

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.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
927 bytes
20.77 KB

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.

tstoeckler’s picture

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.

tstoeckler’s picture

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

tstoeckler’s picture

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

Status: Needs review » Needs work

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

tstoeckler’s picture

Status: Needs work » Needs review

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

tstoeckler’s picture

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.

vijaycs85’s picture

Status: Needs review » Needs work

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

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
10.89 KB

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

dawehner’s picture

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

tim.plunkett’s picture

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

Gábor Hojtsy’s picture

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 :)

Gábor Hojtsy’s picture

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

kim.pepper’s picture

Status: Needs review » Needs work

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

dawehner’s picture

+++ 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?

Gábor Hojtsy’s picture

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.

tstoeckler’s picture

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

tstoeckler’s picture

andypost’s picture

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

This looks like needs API change tag

kim.pepper’s picture

vijaycs85’s picture

Status: Needs work » Needs review

Lets test the last patch...

prateek479’s picture

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.

Gábor Hojtsy’s picture

Issue tags: -sprint
Gábor Hojtsy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Xano’s picture

Status: Needs work » Needs review
FileSize
11.22 KB

Re-roll.

Status: Needs review » Needs work

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

victor-shelepen’s picture

victor-shelepen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

victor-shelepen’s picture

vijaycs85’s picture

Status: Needs work » Needs review

Ok, it is green and needs review.

sun’s picture

Issue tags: -Random fail +Random test failure
YesCT’s picture

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

YesCT’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
11.08 KB

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.

YesCT’s picture

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.

YesCT’s picture

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

YesCT’s picture

  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.

YesCT’s picture

+++ 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().

YesCT’s picture

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.

YesCT’s picture

FileSize
89.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.

YesCT’s picture

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.

YesCT’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.12 KB

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.

alexpott’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
11.1 KB
803 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:

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

andypost’s picture

Looks good to go, but seems there's no tests for that form.

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

This change points that there's no tests for that form.

tim.plunkett’s picture

I think there are tests, just that we don't have any entity types without a label being used here.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue tags: +sprint

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
11.08 KB

Rerolled.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

The missing test concern(only concern on this issue) raised by @andypost at #128 has been addressed by @tim.plunkett at #129. If we need to create and test an entity without label, it can be done as followup as it might not be the scope of this controller conversion issue.

sun’s picture

Issue tags: -Random test failure
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e0212b0 and pushed to 8.x. Thanks!

  • Commit e0212b0 on 8.x by alexpott:
    Issue #1987726 by vijaycs85, disasm, YesCT, pdrake, tstoeckler, dawehner...
Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, so great to see this land finally. Thanks all for your continued effort!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.