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.

Files: 
CommentFileSizeAuthor
#66 language_admin_overview-2005778-66.patch12.06 KBmaggo
PASSED: [[SimpleTest]]: [MySQL] 56,859 pass(es).
[ View ]
#66 interdiff.58-66.txt735 bytesmaggo
#60 language_admin_overview-2005778-60.patch11.92 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 56,781 pass(es), 4 fail(s), and 86 exception(s).
[ View ]
#60 interdiff.txt949 bytesnaxoc
#58 interdiff.54-58.txt1.89 KBpenyaskito
#58 language_admin_overview-2005778-58.patch12.04 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 56,921 pass(es).
[ View ]
#55 editgone.png154.05 KBYesCT
#54 2005778-language-admin-overview-54.patch11.98 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,185 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#54 2005778-diff-52-54.txt748 bytesvijaycs85
#52 2005778-language-admin-overview-52.patch12.03 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 56,788 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#52 2005778-diff-50-52.txt914 bytesvijaycs85
#50 language_admin_overview-2005778-50.patch11.88 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 56,961 pass(es).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] 56,769 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#45 interdiff.43-45.txt7.47 KBpenyaskito
#45 language_admin_overview-2005778-45.patch24.71 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 56,740 pass(es), 37 fail(s), and 0 exception(s).
[ View ]
#43 interdiff.39-43.txt9.25 KBpenyaskito
#43 language_admin_overview-2005778-43.patch17.23 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 56,711 pass(es), 58 fail(s), and 0 exception(s).
[ View ]
#39 interdiff.txt1.33 KBandypost
#39 language_admin_overview-2005778-39.patch10.61 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,746 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#38 interdiff.txt5.5 KBandypost
#38 language_admin_overview-2005778-38.patch10.61 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,747 pass(es), 21 fail(s), and 207 exception(s).
[ View ]
#37 language_admin_overview-2005778-36.patch10.74 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 56,667 pass(es).
[ View ]
#37 language_admin_overview-2005778-32-36.interdiff.txt572 byteswebflo
#32 language_admin_overview-2005778-32.patch10.81 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 56,809 pass(es).
[ View ]
#32 language_admin_overview-2005778-25-32.interdiff.txt7.74 KBwebflo
#25 language_admin_overview-2005778-25.patch8.36 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 56,820 pass(es), 4 fail(s), and 164 exception(s).
[ View ]
#25 interdiff.txt7.44 KBnaxoc
#19 language_admin_overview-2005778-19.patch7.12 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 56,450 pass(es).
[ View ]
#19 language_admin_overview-2005778-19.interdiff.txt787 byteswebflo
#13 language_admin_overview-2005778-11.patch7.13 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 57,969 pass(es).
[ View ]
#11 language_admin_overview-2005778-11.patch7.13 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#7 language_admin_overview-2005778-7.patch7.08 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 58,612 pass(es).
[ View ]
#5 language_admin_overview-2005778-5.patch7.42 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#5 interdiff.txt1013 bytesnaxoc
#3 language_admin_overview-2005778-2.patch7.08 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 58,089 pass(es).
[ View ]
#1 language_admin_overview-2005778-1.patch7.08 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 57,788 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new7.08 KB
PASSED: [[SimpleTest]]: [MySQL] 57,788 pass(es).
[ View ]

First approach.

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

StatusFileSize
new7.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,089 pass(es).
[ View ]

Here is a reroll of #1

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?

StatusFileSize
new1013 bytes
new7.42 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

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

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!

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new7.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,612 pass(es).
[ View ]

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

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.

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

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

Status:Needs work» Needs review
StatusFileSize
new7.13 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Converted check_plain to String::checkPlain().

Status:Needs review» Reviewed & tested by the community

Looks good!

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new7.13 KB
PASSED: [[SimpleTest]]: [MySQL] 57,969 pass(es).
[ View ]

Testbot got stuck. We test it again.

Status:Needs review» Reviewed & tested by the community

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

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

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

Assigned:plopesc» Unassigned

Unassign

Status:Needs work» Needs review
StatusFileSize
new787 bytes
new7.12 KB
PASSED: [[SimpleTest]]: [MySQL] 56,450 pass(es).
[ View ]

Rerolled.

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.

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.

Assigned:Unassigned» naxoc

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.

Status:Needs work» Needs review
StatusFileSize
new7.44 KB
new8.36 KB
FAILED: [[SimpleTest]]: [MySQL] 56,820 pass(es), 4 fail(s), and 164 exception(s).
[ View ]

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.

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

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.

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

public function load() {
  return $this->storage->loadByProperties(array('locked' => '0'));
}

Wow, fancy!

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?

Status:Needs work» Needs review
StatusFileSize
new7.74 KB
new10.81 KB
PASSED: [[SimpleTest]]: [MySQL] 56,809 pass(es).
[ View ]

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.

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?

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.

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'

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

StatusFileSize
new572 bytes
new10.74 KB
PASSED: [[SimpleTest]]: [MySQL] 56,667 pass(es).
[ View ]

Okay i got it.

StatusFileSize
new10.61 KB
FAILED: [[SimpleTest]]: [MySQL] 56,747 pass(es), 21 fail(s), and 207 exception(s).
[ View ]
new5.5 KB

That's should be better

StatusFileSize
new10.61 KB
FAILED: [[SimpleTest]]: [MySQL] 56,746 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
new1.33 KB

Reverted back to languages because of locale_form_language_admin_overview_form_alter()

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

Assigned:naxoc» penyaskito

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

Status:Needs work» Needs review
StatusFileSize
new17.23 KB
FAILED: [[SimpleTest]]: [MySQL] 56,711 pass(es), 58 fail(s), and 0 exception(s).
[ View ]
new9.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.

Status:Needs work» Needs review
StatusFileSize
new24.71 KB
FAILED: [[SimpleTest]]: [MySQL] 56,740 pass(es), 37 fail(s), and 0 exception(s).
[ View ]
new7.47 KB

Finishing url conversion.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new25.33 KB
FAILED: [[SimpleTest]]: [MySQL] 56,769 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new985 bytes

Oops, missed one.

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.

Status:Needs work» Needs review
StatusFileSize
new5.02 KB
new11.88 KB
PASSED: [[SimpleTest]]: [MySQL] 56,961 pass(es).
[ View ]

Based on patch from comment 37.

- Fixed the drag and drop problem
- Added LanguageAccessController

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

StatusFileSize
new914 bytes
new12.03 KB
FAILED: [[SimpleTest]]: [MySQL] 56,788 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

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

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

StatusFileSize
new748 bytes
new11.98 KB
FAILED: [[SimpleTest]]: [MySQL] 57,185 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

:) same comment from @andypost at #2031277-3: Implement checkCreateAccess on LanguageAccessController. Updating here.

Status:Needs review» Needs work
StatusFileSize
new154.05 KB

edit is missing from the languages table.

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

editgone.png

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.

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?

Status:Needs work» Needs review
StatusFileSize
new12.04 KB
PASSED: [[SimpleTest]]: [MySQL] 56,921 pass(es).
[ View ]
new1.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.

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

StatusFileSize
new949 bytes
new11.92 KB
FAILED: [[SimpleTest]]: [MySQL] 56,781 pass(es), 4 fail(s), and 86 exception(s).
[ View ]

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.

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.

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

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.

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

StatusFileSize
new735 bytes
new12.06 KB
PASSED: [[SimpleTest]]: [MySQL] 56,859 pass(es).
[ View ]

Added create operation in LanguageAccessController, based on #58

Status:Needs work» Needs review

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

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

Status:Reviewed & tested by the community» Fixed

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

Issue tags:-sprint

Yay!

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