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
#40 drupal-convert_locale_change-1978928-40.patch5.02 KBpiyuesh23
PASSED: [[SimpleTest]]: [MySQL] 57,168 pass(es).
[ View ]
#40 interdiff.txt671 bytespiyuesh23
#36 interdiff.txt1.51 KBpiyuesh23
#35 drupal-convert_locale_change-1978928-35.patch4.98 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] 56,812 pass(es), 0 fail(s), and 16 exception(s).
[ View ]
#35 interdiff.txt1.51 KBpiyuesh23
#31 drupal-convert_locale_change-1978928-31.patch4.86 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] 57,180 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#31 interdiff.txt1.23 KBpiyuesh23
#28 Available translation updates | drupal8.png24.8 KBrahul.shinde
#26 drupal-convert_locale_change-1978928-26.patch4.76 KBpiyuesh23
PASSED: [[SimpleTest]]: [MySQL] 56,626 pass(es).
[ View ]
#26 interdiff.txt729 bytespiyuesh23
#23 drupal-convert_locale_change-1978928-20.patch4.75 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] 56,624 pass(es), 50 fail(s), and 0 exception(s).
[ View ]
#23 interdiff.txt1.24 KBpiyuesh23
#20 drupal-convert_locale_change-1978928-20.patch4.75 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] 56,543 pass(es), 50 fail(s), and 0 exception(s).
[ View ]
#20 interdiff.patch1.24 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 locale-change_callback_to_controller-1978928-15.patch4.69 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] 55,159 pass(es), 965 fail(s), and 139 exception(s).
[ View ]
#15 drupal-convert_locale_change-1978928-8.patch4.73 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 56,117 pass(es), 66 fail(s), and 28 exception(s).
[ View ]
#12 Screen Shot 2013-06-29 at 12.49.48 PM.png186.99 KBpiyuesh23
#8 locale-change_callback_to_controller-1978928-8.patch4.65 KBInternetDevels
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-change_callback_to_controller-1978928-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 interdiff.txt548 bytesInternetDevels
#6 locale-change_callback_to_controller-1978928-6.patch4.65 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 55,308 pass(es).
[ View ]
#6 interdiff.txt2.39 KBInternetDevels
#4 locale-change_callback_to_controller-1978928-4.patch4.38 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 56,047 pass(es).
[ View ]

Comments

Assigned:Unassigned» Pancho
Issue tags:+D8MI

Assigned:Pancho» InternetDevels

As this issue hasn`t been fixed yet we are going to work today with this issue during Code Sprint UA.

That's fine!

Assigned:InternetDevels» Unassigned
Status:Active» Needs review
Issue tags:+CodeSprintUA
StatusFileSize
new4.38 KB
PASSED: [[SimpleTest]]: [MySQL] 56,047 pass(es).
[ View ]

Patch attached.

Status:Needs review» Needs work

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -0,0 +1,63 @@
+  /**
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   */
+  public function __construct(ModuleHandlerInterface $module_handler) {

The construct method should be documented

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -0,0 +1,63 @@
+   * Manually checks the translation status without the use of cron.
+   */
+  public function check_translation() {

This should have a @return documentation. Let's also use a camelCase method here.

+++ b/core/modules/locale/locale.moduleundefined
@@ -232,10 +232,8 @@ function locale_menu() {
   $items['admin/reports/translations/check'] = array(
     'title' => 'Manual translation update check',
-    'page callback' => 'locale_translation_manual_status',
-    'access arguments' => array('translate interface'),
     'type' => MENU_CALLBACK,
-    'file' => 'locale.pages.inc',
+    'route_name' => 'locale_check_translation',

If you have just a callback you don't need an entry in hook_menu anymore.

Status:Needs work» Needs review
StatusFileSize
new2.39 KB
new4.65 KB
PASSED: [[SimpleTest]]: [MySQL] 55,308 pass(es).
[ View ]

Here is the new patch.

+++ b/core/modules/locale/locale.routing.ymlundefined
@@ -4,3 +4,10 @@ locale_settings:
+    _content: 'Drupal\locale\Controller\LocaleController::checkTranslation'

If you just do a redirect response I would say that _controller is the better choice as _content

That's the last bit, I promise :)

StatusFileSize
new548 bytes
new4.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-change_callback_to_controller-1978928-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And finally (I hope so :))...

Status:Needs review» Needs work

The last submitted patch, locale-change_callback_to_controller-1978928-8.patch, failed testing.

Assigned:Unassigned» dutchyoda

Assigned:dutchyoda» Unassigned

Status:Needs work» Needs review
StatusFileSize
new186.99 KB

Tested on my local and the patch works fine. Ran the forum test case through simpletest module on my local. Attaching a screenshot for more info.
Queuing this issue for re-testing.

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

The last submitted patch, locale-change_callback_to_controller-1978928-8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.73 KB
FAILED: [[SimpleTest]]: [MySQL] 56,117 pass(es), 66 fail(s), and 28 exception(s).
[ View ]

reroll!

Status:Needs review» Needs work

The last submitted patch, drupal-convert_locale_change-1978928-8.patch, failed testing.

StatusFileSize
new4.69 KB
FAILED: [[SimpleTest]]: [MySQL] 55,159 pass(es), 965 fail(s), and 139 exception(s).
[ View ]

The failed patch was missing the route_name property. Added it back. Attaching the fixed patch.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, locale-change_callback_to_controller-1978928-15.patch, failed testing.

StatusFileSize
new1.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new4.75 KB
FAILED: [[SimpleTest]]: [MySQL] 56,543 pass(es), 50 fail(s), and 0 exception(s).
[ View ]

Fixing the patch. Updated the include path for couple of components and added route_name property. Attaching the patch and re-queuing it for testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, interdiff.patch, failed testing.

StatusFileSize
new1.24 KB
new4.75 KB
FAILED: [[SimpleTest]]: [MySQL] 56,624 pass(es), 50 fail(s), and 0 exception(s).
[ View ]

Renaming interdiff.patch to interdiff.txt.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-convert_locale_change-1978928-20.patch, failed testing.

StatusFileSize
new729 bytes
new4.76 KB
PASSED: [[SimpleTest]]: [MySQL] 56,626 pass(es).
[ View ]

Controller was missing a return statement for the batch process. Tests might be failing coz of this. Attaching another patch and requeing for testing.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new24.8 KB

Patch provided in #26 is working as expected. Please find attachment (Available translation updates | drupal8.png) for better understanding.

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -0,0 +1,69 @@
+    return new RedirectResponse(url('admin/reports/translations', array('absolute' => TRUE)));

Need to inject the url generator.

+++ b/core/modules/locale/locale.moduleundefined
@@ -217,10 +217,8 @@ function locale_menu() {
   $items['admin/reports/translations/check'] = array(
     'title' => 'Manual translation update check',
-    'page callback' => 'locale_translation_manual_status',
-    'access arguments' => array('translate interface'),
     'type' => MENU_CALLBACK,
-    'file' => 'locale.pages.inc',
+    'route_name' => 'locale_check_translation',
   );

Let's remove the entire hook menu... a user should never actually land on the page as the controller does a redirect to admin/reports/translations

StatusFileSize
new1.23 KB
new4.86 KB
FAILED: [[SimpleTest]]: [MySQL] 57,180 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

@alexpott

Thanks for the review, makes sense to remove the menu hook completely..:)

Updating the patch to inject the url generator properly and removing the menu item.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-convert_locale_change-1978928-31.patch, failed testing.

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -0,0 +1,70 @@
+    $url = $this->urlGenerator->generateFromPath('admin/reports/translations', array('absolute' => TRUE));

You're missing $this->urlGenerator

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -0,0 +1,70 @@
+    $url = $this->urlGenerator->generateFromPath('admin/reports/translations', array('absolute' => TRUE));
+    return new RedirectResponse($url);

While rerolling, can you just make this one line? return new RedirectResponse($this->urlGenerator->...

StatusFileSize
new1.51 KB
new4.98 KB
FAILED: [[SimpleTest]]: [MySQL] 56,812 pass(es), 0 fail(s), and 16 exception(s).
[ View ]

@tim Thanks. Uploading a patch with the above fixes.

StatusFileSize
new1.51 KB

@tim Thanks. Uploading a patch with the above fixes.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-convert_locale_change-1978928-35.patch, failed testing.

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -0,0 +1,71 @@
+    $this->urlGenerator = $url_generator;
...
+    return new static(
+      $container->get('module_handler')

Well, you actually need to pass in the generator service.

StatusFileSize
new671 bytes
new5.02 KB
PASSED: [[SimpleTest]]: [MySQL] 57,168 pass(es).
[ View ]

@tim Thanks for helping out on this. Updated the patch.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

This looks fine.

Status:Reviewed & tested by the community» Fixed

Committed 2b08f2e and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

Issue tags:+language-ui