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.

CommentFileSizeAuthor
#66 language_admin_overview-2005778-66.patch12.06 KBmaggo
#66 interdiff.58-66.txt735 bytesmaggo
#60 language_admin_overview-2005778-60.patch11.92 KBnaxoc
#60 interdiff.txt949 bytesnaxoc
#58 interdiff.54-58.txt1.89 KBpenyaskito
#58 language_admin_overview-2005778-58.patch12.04 KBpenyaskito
#55 editgone.png154.05 KBYesCT
#54 2005778-language-admin-overview-54.patch11.98 KBvijaycs85
#54 2005778-diff-52-54.txt748 bytesvijaycs85
#52 2005778-language-admin-overview-52.patch12.03 KBvijaycs85
#52 2005778-diff-50-52.txt914 bytesvijaycs85
#50 language_admin_overview-2005778-50.patch11.88 KBwebflo
#50 language_admin_overview-2005778-37-50.interdiff.txt5.02 KBwebflo
#47 interdiff.45-47.txt985 bytespenyaskito
#47 language_admin_overview-2005778-47.patch25.33 KBpenyaskito
#45 interdiff.43-45.txt7.47 KBpenyaskito
#45 language_admin_overview-2005778-45.patch24.71 KBpenyaskito
#43 interdiff.39-43.txt9.25 KBpenyaskito
#43 language_admin_overview-2005778-43.patch17.23 KBpenyaskito
#39 interdiff.txt1.33 KBandypost
#39 language_admin_overview-2005778-39.patch10.61 KBandypost
#38 interdiff.txt5.5 KBandypost
#38 language_admin_overview-2005778-38.patch10.61 KBandypost
#37 language_admin_overview-2005778-36.patch10.74 KBwebflo
#37 language_admin_overview-2005778-32-36.interdiff.txt572 byteswebflo
#32 language_admin_overview-2005778-32.patch10.81 KBwebflo
#32 language_admin_overview-2005778-25-32.interdiff.txt7.74 KBwebflo
#25 language_admin_overview-2005778-25.patch8.36 KBnaxoc
#25 interdiff.txt7.44 KBnaxoc
#19 language_admin_overview-2005778-19.patch7.12 KBwebflo
#19 language_admin_overview-2005778-19.interdiff.txt787 byteswebflo
#13 language_admin_overview-2005778-11.patch7.13 KBwebflo
#11 language_admin_overview-2005778-11.patch7.13 KBwebflo
#7 language_admin_overview-2005778-7.patch7.08 KBnaxoc
#5 language_admin_overview-2005778-5.patch7.42 KBnaxoc
#5 interdiff.txt1013 bytesnaxoc
#3 language_admin_overview-2005778-2.patch7.08 KBnaxoc
#1 language_admin_overview-2005778-1.patch7.08 KBplopesc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

Status: Active » Needs review
FileSize
7.08 KB

First approach.

naxoc’s picture

I'll take a look at this today at the Dev Days sprint.

naxoc’s picture

Here is a reroll of #1

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint, +language-base

Patch looks pretty good. The code moved around seems identical.

+++ b/core/modules/language/lib/Drupal/language/Form/LanguageAdminOverviewForm.phpundefined
@@ -0,0 +1,107 @@
+/**
+ * User interface for the language overview screen.
+ */
+class LanguageAdminOverviewForm implements FormInterface {

Should this also implement a controller interface (or extend from a base form controller) since it is a form controller after all? :) I mean not just implement this interface?

naxoc’s picture

Updated patch. The form controller now inherits from a controller interface.

Would it make better sense to make a base form controller for it?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looked through other FormInterface implementations. This seems like well enough based on those. There is also no need to inject something here given no outside services used. So looks good!

naxoc’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.08 KB

Just talked to Gabor and we found that the implementation of a controller interface is not necessary after all.

This is just the patch from #3

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

@katbailey also explains we only need the controller interface implemented if we are to inject something and as per #6 we don't have. So #3 should be fine.

Gábor Hojtsy’s picture

Jup, #7 just a repost of #3, still RTBC :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/lib/Drupal/language/Form/LanguageAdminOverviewForm.phpundefined
@@ -0,0 +1,107 @@
+        '#markup' => check_plain($language->name),

We shouldn't be using check_plain here... String::checkPlain()

webflo’s picture

Status: Needs work » Needs review
FileSize
7.13 KB

Converted check_plain to String::checkPlain().

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

webflo’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.13 KB

Testbot got stuck. We test it again.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
tim.plunkett’s picture

+++ b/core/modules/language/lib/Drupal/language/Form/LanguageAdminOverviewForm.phpundefined
@@ -0,0 +1,108 @@
+class LanguageAdminOverviewForm implements FormInterface {

When #1754246: Languages should be configuration entities goes in, this should actually be a EntityListController. See RoleListController as an example.

Gábor Hojtsy’s picture

We are aware of that, discussed with @alexpott too.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This now needs to be an entity list controller as #1754246: Languages should be configuration entities has landed

plopesc’s picture

Assigned: plopesc » Unassigned

Unassign

webflo’s picture

Status: Needs work » Needs review
FileSize
787 bytes
7.12 KB

Rerolled.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

As said above, now that #1754246: Languages should be configuration entities is in, this patch should extend from entity list controller and don't produce the data for the list itself. Much like any other entity list controller in contact, roles, etc.

andypost’s picture

Gábor Hojtsy’s picture

This unfortunately also does not use the buildHeader(), getOperations(), buildOperations(), etc. patter that is required to support #2004428: Less ugly operations altering in config_translation. It should use the proper method separations that are in config entity list controllers generally.

naxoc’s picture

Assigned: Unassigned » naxoc
tim.plunkett’s picture

Actually, this isn't listing Drupal\language\Plugin\Core\Entity\Language, which would use the EntityListController.
It's actually listing Drupal\Core\Language\Language objects.

naxoc’s picture

Status: Needs work » Needs review
FileSize
7.44 KB
8.36 KB

Here is a patch that uses EntityListController.

It looks to me like it lists Drupal\language\Plugin\Core\Entity\Language - I had no trouble with listing them.

Status: Needs review » Needs work

The last submitted patch, language_admin_overview-2005778-25.patch, failed testing.

tim.plunkett’s picture

+++ /dev/nullundefined
@@ -1,108 +0,0 @@
-    $languages = language_list();

This is a list of Drupal\Core\Language\Language objects

+++ b/core/modules/language/lib/Drupal/language/LanguageListController.phpundefined
@@ -0,0 +1,137 @@
+  public function buildRow(EntityInterface $entity) {

This is Drupal\language\Plugin\Core\Entity\Language.

When I looked at this earlier, it would show zxx and und languages as well.

Gábor Hojtsy’s picture

Tim is right that locked languages should not be shown. Rows where locked = 1 should be skipped as per prior to this patch. Also, the submission function should not use language_list(), so it is self contained in terms of the entity management it does. language_list() works with a processed list of languages, the form submission should work with entities direct.

tim.plunkett’s picture

You can override the EntityListController::load() like this:

public function load() {
  return $this->storage->loadByProperties(array('locked' => '0'));
}
Gábor Hojtsy’s picture

Wow, fancy!

naxoc’s picture

I am having trouble saving the weights on the languages without using language_list().

I tried calling save() on the entities, but then the names disappear.

What would be the best way to save the weights?

webflo’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
10.81 KB

Hmm i can not reproduce this problem. But its required to kill the static cache in language_list and call language_update_locked_weights() to updated the non configurable languages.

penyaskito’s picture

I'm wondering if we could refactor language_save() and this controller, so we shouldn't need to duplicate the cache-clearing + updating of locked weights. That could be done in a follow-up and I wouldn't consider that a blocker for RTBCing.

+++ b/core/modules/language/lib/Drupal/language/LanguageListController.phpundefined
@@ -0,0 +1,171 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function validateForm(array &$form, array &$form_state) {
+    // No validation.
+  }

If there is no validation, could we remove this?

penyaskito’s picture

Answering myself: validateForm is needed for implementing FormInterface and not provided for any parent classes.
I would mark this as RTBC, but would be great having other two eyes.

andypost’s picture

Otherwise good

+++ b/core/modules/language/language.routing.ymlundefined
@@ -18,3 +18,11 @@ language_negotiation_selected:
+    _content: '\Drupal\Core\Entity\Controller\EntityListController::listing'
+    entity_type: 'language_entity'

_entity_list = 'language_entity'

webflo’s picture

So we should add both? EntityListController::listing needs $entity_type as argument.

webflo’s picture

andypost’s picture

That's should be better

andypost’s picture

Reverted back to languages because of locale_form_language_admin_overview_form_alter()

webflo’s picture

+++ b/core/modules/language/lib/Drupal/language/LanguageListController.phpundefined
@@ -0,0 +1,154 @@
+    $operations = parent::getOperations($entity);

+++ b/core/modules/language/lib/Drupal/language/Plugin/Core/Entity/Language.phpundefined
@@ -76,4 +77,17 @@ class Language extends ConfigEntityBase implements LanguageInterface {
+      'path' => 'admin/config/regional/language/' . $this->id(),

Langauge entities dont use the common URL structure form Drupal core (entity/{id}/edit). I makes no sense to use uri() and generated edit/delete links at this point. We can convert this urls in a follow-up.

Status: Needs review » Needs work

The last submitted patch, language_admin_overview-2005778-39.patch, failed testing.

penyaskito’s picture

Assigned: naxoc » penyaskito

I'm working on a fix for this, patch will follow.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
17.23 KB
9.25 KB

My first try to get it back to green finished with something very similar to #43, so I discarded and tried something different.

For reusing buildOperations, uri() and access() were required in the entity.
In the case of configuration entities, default edit path is the MENU_DEFAULT_LOCAL_TASK but in this case IMHO does not make sense, so I had to workaround it. For making this work was necessary to modify edit and delete paths in hook_menu, but I think that this is something desired anyway sooner or later for consistency with other entities.

In locale_form_language_admin_overview_form_alter we have to ensure that the weight is the last column, but anyway the toggle of visibility of the weight column is not working because the #links in operations. I couldn't find an issue for that bug, but IMHO this should be fixed instead of doing workarounds.

-    // #type = link doesn't work with #weight on table.
-    // reset and set it back after locale_statistics to get it at the right end.
-    $operations = $form['languages'][$langcode]['operations'];
-    unset($form['languages'][$langcode]['operations']);
-    $form['languages'][$langcode]['operations'] = $operations;

Status: Needs review » Needs work

The last submitted patch, language_admin_overview-2005778-43.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
24.71 KB
7.47 KB

Finishing url conversion.

Status: Needs review » Needs work

The last submitted patch, language_admin_overview-2005778-45.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
25.33 KB
985 bytes

Oops, missed one.

Gábor Hojtsy’s picture

All (almost all?) other config entities in core now use the ...../manage/.... pattern in the paths. It is true languages don't do that. If we *need* to change the paths here, we should rather change to that. If we can avoid changing the paths for the controller, that would probably be better just for the sake of making it easier to review.

Status: Needs review » Needs work

The last submitted patch, language_admin_overview-2005778-47.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
5.02 KB
11.88 KB

Based on patch from comment 37.

- Fixed the drag and drop problem
- Added LanguageAccessController

andypost’s picture

Take example form menu access patch #2012916: Implement access controller for the menu and menu link entity

+++ b/core/modules/language/lib/Drupal/language/LanguageAccessController.phpundefined
@@ -0,0 +1,24 @@
+  public function access(EntityInterface $entity, $operation, $langcode = Language::LANGCODE_DEFAULT, AccountInterface $account = NULL) {
+    return !$entity->locked && user_access('administer languages', $account);

this should be $operation specific

vijaycs85’s picture

Added LanguageAccessController in #2031277: Implement checkCreateAccess on LanguageAccessController. Same code updated here.

penyaskito’s picture

+++ b/core/modules/language/lib/Drupal/language/LanguageAccessController.phpundefined
@@ -18,7 +18,14 @@ class LanguageAccessController extends EntityAccessController {
+        $language = language_load($entity->id);

The $entity is already the $language, why loading it again?

vijaycs85’s picture

YesCT’s picture

Status: Needs review » Needs work
FileSize
154.05 KB

edit is missing from the languages table.

I looked around, but haven't figured out why yet.

editgone.png

YesCT’s picture

While reading this, I'd recommend these changes (if they are accurate).

@@ -38,8 +38,8 @@ public function getOperations(EntityInterface $entity) {
     $operations = parent::getOperations($entity);
     $default = language_default();
 
-    // Edit and delete path for Languages entities have a different scheme
-    // then other config entities.
+    // Edit and delete path for Languages entities have a different pattern
+    // than other config entities.
     $path = 'admin/config/regional/language';
     if (isset($operations['edit'])) {
       $operations['edit']['href'] = $path . '/edit/' . $entity->id();
@@ -48,6 +48,7 @@ public function getOperations(EntityInterface $entity) {
       $operations['delete']['href'] = $path . '/delete/' . $entity->id();
     }
 
+    // Deleting the site default language is not allowed.
     if ($entity->id() == $default->id) {
       unset($operations['delete']);
     }

scheme momentarily confused me with schema. :) so I think we can just say pattern.
And "then" is about time. "than" is a comparison. So we want "than" here.

Some comment about why the site default is has a special check would be good.

naxoc’s picture

I am wondering why we call the parent getOperations, buildHeader(), and buildRow just to remove stuff from the returned values promptly? It feels to me like we loose control over what we display if the parent decides to throw in stuff we don't want?

penyaskito’s picture

Status: Needs work » Needs review
FileSize
12.04 KB
1.89 KB

Edit should be available now, the permission is "update" and not "edit".
Fixed comments said in #56.

@naxoc: We are only unsetting the id because it's not very user-friendly to show the langcode in this case, and instead we have the language name.

tim.plunkett’s picture

We need to call buildOperations() for the hook_entity_operations_alter() to fire.
The rest, no need to call parent.

naxoc’s picture

Here is a patch that does nothing but remove the unnecessary calls to parent().

Status: Needs review » Needs work

The last submitted patch, language_admin_overview-2005778-60.patch, failed testing.

penyaskito’s picture

If we remove the calls to parent, we need to include the operations from there. I consider #58 much cleaner than adding this code duplication.

YesCT’s picture

+++ b/core/modules/language/lib/Drupal/language/LanguageAccessController.phpundefined
@@ -19,7 +19,7 @@ class LanguageAccessController extends EntityAccessController {
     switch ($operation) {
-      case 'edit':
+      case 'update':
       case 'delete':

+++ b/core/modules/language/lib/Drupal/language/LanguageListController.phpundefined
@@ -38,8 +38,8 @@ public function getOperations(EntityInterface $entity) {
     if (isset($operations['edit'])) {
       $operations['edit']['href'] = $path . '/edit/' . $entity->id();

in access() are these operations, or permissions? It is confusing because in other places, the operation is edit.

====
Seems like we can either use parent, and then remove stuff,
or
not use parent, and add specifically the stuff from parent that we want.

If, something is added to parent that we want to inherit, and we dont use parent, we have to update things here to, so I think would would need an @see in the parent pointing to this, so we know when that changes that we should change things here?

Then I suppose the opposite is true too.

Do we have another place in core for something similar happening that we can use as a guide for what approach to take?
Maybe using the parent pattern is already a common way to deal with this situation.

Also.. #58 was green.

penyaskito’s picture

The operations for the access methods are create, update, delete. I checked CommentAccessController for another example.

About another example, in VocabularyListController you can find that we are using the same strategy than here.

YesCT’s picture

Yep. I checked MenuListController and MenuAccessController too. it's update operation in access and edit operation in list. :)

maggo’s picture

Added create operation in LanguageAccessController, based on #58

maggo’s picture

Status: Needs work » Needs review
webflo’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. The new access controller for language entities is solid and we can continue with other language admin pages after this got committed.

- #2038291: Convert language_admin_add_form to a Controller
- #1946426: Convert all of confirm_form() in language.admin.inc to the new form interface
- #2003592: Convert language_admin_add_form and language_admin_edit_form to a Controllers

YesCT’s picture

YesCT’s picture

I can confirm that with this applied, that the config_translation module puts a Translate link in the operations of
admin/config/regional/language

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9add7cb and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay!

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

penyaskito’s picture

Assigned: penyaskito » Unassigned
Issue summary: View changes