Language admin form: move top-level language properties to table form row

Essentially, the idea is to make this patch work.

Please do not re-test this patch. It is expected to fail.

Sounds like a cool Novice task so adding the tag and some specific instructions.

The code to update (on top of the patch above) is in language_admin_overview_submit():

• Get $languages from $form_state instead of calling language_list()
• $old_default can be removed entirely

and in locale_form_language_admin_overview_form_alter():

• Get $languages from $form_state instead of $form

There might be something more so adding/editing/moving languages (also with locale.module turned on) should be tried to verify everything works and no notices pop-up.

It seems that 'language_default' is actually not used at all in core. (That would have to be verified by a patch.) So maybe it would be fine to remove it alltogether?

Sorry, I created another follow up issue for #1877338: Convert language admin form to new #type 'table' which follows other approach.

#1882448: Decouple language and locale modules in language_admin_overview_form() tries to decouple the form definition and the form_alter function. Then it is not necessary add extra parameters to the $form or $form_state variables, which are not used if locale module is disabled.

Please, decide which is better and cancel the other one.

Regards

Yeah, no wonder the patch fails locale module uses the language list value at least.

tagging

Screenshot of /admin/config/regional/language

Steps:

1. Enable 'Language' module
2. Go to /admin/config/regional/language

Here's a patch to implement solution in #3.

Adding/editing/deleting/moving works well after patch.

But the default screen now shows a row each for 'Not specified' (/admin/config/regional/language/edit/und) and 'Not applicable' (/admin/config/regional/language/edit/zxx) as languages, and if you try to edit or delete you get Access Denied.

Screenshot-

tagging.

I think the current state of this issue is waiting on a patch that keeps the system languages hidden.

System languages are definitely not there anymore. As per http://api.drupal.org/api/drupal/core%21modules%21language%21language.ad... this patch would be still needed.

We need use language_list without parameters because

function language_list($flags = LANGUAGE_CONFIGURABLE) { ... }

It is enough.

We can also convert this form to the OOP style.

Needs reroll

Rerolling the patch #12

Rerolled this patch and fixed the syntax error in the patch.

Sorry wrong patch file uploaded by mistake. Uploading the patch with fixed syntax error.

I've the impression that this entire issue + patch is obsolete, since the language overview form is not an entity listing form, because languages are configuration entities now?

That said, LanguageListController::buildForm() still appears to contain a bit of the offending code:

  public function buildForm(array $form, array &$form_state) {
    $form = parent::buildForm($form, $form_state);
    $form[$this->entitiesKey]['#languages'] = $this->entities; # <--- THIS
    $form['actions']['submit']['#value'] = t('Save configuration');
    return $form;
  }

However, grepping the Language module code, I'm not able to locate any uses that would be related to that form element property.

So perhaps, let's simply see what happens if we remove it...

This is where its used:

function locale_form_language_admin_overview_form_alter(&$form, &$form_state) {
  $languages = $form['languages']['#languages'];

This code may also take the languages from the language manager, if we assume that should be the same list (which should be :D).

cleaning up assignees, so that Novices will try to get them =)

I will try to help with this issue.

I changed the way, how $languages are retrieved in locale_form_language_admin_overview_form_alter(), but i am not sure if i am doing it the right way.

Hello kim.pepper,

thank you for your advice. I added my interdiff.txt file now.

Mario

Thanks, @dermario! :)

While the code could manually call into LanguageManager to re-retrieve a list of languages, doing so would introduce a possibility for operating on different lists of languages, and thus, potential bugs later on.

So this is interesting... unless I missed something big, there is no way to retrieve the already loaded entities from the DraggableListController?

$form_state['build_info']['callback_object'] exists and should hold the LanguageListController.

→ Apparently, there's no getter method to retrieve the controller, just a raw array key in $form_state?

DraggableListController::$entities exists, but is protected.

→ Neither DraggableListController nor EntityListController provides a getEntities() method?

The latter is needed here, so this code is able to do:

$languages = $form_state['build_info']['callback_object']->getEntities();

Anyone any ideas?

The patch in comment #28 doesn't apply, because the file core/modules/language/lib/Drupal/language/LanguageListController.php doesn't exist.

The languages can be retrieved with the 'load' method in the $form_state['build_info']['callback_object'], which is a LanguageListBuilder object. This functions as a 'getEntities' method.

Added a patch as a solution for https://drupal.org/comment/8417017#comment-8417017

As far as I can tell the previous patch doesn't address the open issue directly, which itself doesn't seem to have anything todo with the original issue title anymore.

In #22 sun states that the issue is resolved, but that we need to get rid of $form[$this->entitiesKey]['#languages'] = $this->entities; in

<?php
  public function buildForm(array $form, array &$form_state) {
    $form = parent::buildForm($form, $form_state);
    $form[$this->entitiesKey]['#languages'] = $this->entities; # <--- THIS
    $form['actions']['submit']['#value'] = t('Save configuration');
    return $form;
  }
?>

(a reference would be useful to why exactly we need to get rid it)

As Gabor mentions in #24, the entities are needed within locale_form_language_admin_overview_form_alter. So the real issue now is how to get the languages in locale_form_language_admin_overview_form_alter.

Gabor suggests

This code may also take the languages from the language manager, if we assume that should be the same list (which should be :D).

So that was done in #28

In contradiction to this sun then suggests

While the code could manually call into LanguageManager to re-retrieve a list of languages, doing so would introduce a possibility for operating on different lists of languages, and thus, potential bugs later on.

If we follow this then we need to be able to fetch the already loaded language entities. But the patch in [#8417017-33] uses load in LanguageListBuilder which will also re-load the entities. I'm not sure if this is in the end any different from the patch in #28 which uses the language manager.

So we either A) reload the language entities or B) we need to introduce getEntities (as sun suggests in #31) in DraggableListBuilder (as the $entities member variable is being defined there), which will then return the cached entities.

I'll start implementing getEntities to check if this would work.

Patch #28 would need a reroll due to the renaming of ListController to ListBuilder. Based on this I will also update the issue summary.

major summary update

minor changes

typo in numbers in summary. ignore this.

@ti2m, are you still working of this patch?
Removing 'novice' tag. I believe it is no longer applicable.

Yes, patch is ready, just need to create it.

Here the patch to implement getEntities() in DraggableListBuilder to retrieve the cached entities. It's a minimalistic patch, submitForm could use getEntities() as well instead of directly accessing the entities.

Patch #41 failed because SearchPageListBuilder (extending DraggableListBuilder) defined its own $entities property and initialized it with an empty array, instead of NULL which indicates that the entites were never loaded. As SearchPageListBuilder is doing that for no obvious reason I removed it.

+++ b/core/lib/Drupal/Core/Config/Entity/DraggableListBuilder.php
@@ -116,8 +126,8 @@ public function buildForm(array $form, array &$form_state) {
+    $entities = $this->getEntities();
+    foreach ($entities as $entity) {

Minor, but could be collapsed into one line (i.e. (foreach ($this->getEntities() ...)))

I also think it would make sense to switch to getEntities() everywhere else directly in this issue, i.e. in submitForm() as you mentioned above.

Most importantly, though:
@ti2m as you spent a significant amount of time reading this issue, and now in contrast to me and I think many other people, know what this issue is about could you update the issue summary and title with what is now being done here? That would be incredibly helpful.

related
#2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface
and maybe other sub issues of #2239497: [Meta] Fix ConfigurableLanguageManager getLanguages()

[edit: oops. might not be appropriate here. I might have had the wrong issue.]

I have re-rolled the patch to work with commit f678e37.

There were conflicts in the following files that I resolved manually:

core\lib\Drupal\Core\Config\Entity\DraggableListBuilder.php - lines 130-139

core\modules\language\src\LanguageListBuilder.php - moved from core/modules/language/lib/Drupal/language/LanguageListBuilder.php

I have tested the page admin/config/regional/language and this appears to work. It looks the same before and after the patch.