Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

Assigned: Unassigned » Pancho
Issue tags: +D8MI
InternetDevels’s picture

Assigned: Pancho » InternetDevels

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

Pancho’s picture

That's fine!

InternetDevels’s picture

Assigned: InternetDevels » Unassigned
Status: Active » Needs review
Issue tags: +CodeSprintUA
FileSize
4.38 KB

Patch attached.

dawehner’s picture

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.

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
4.65 KB

Here is the new patch.

dawehner’s picture

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

InternetDevels’s picture

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

Status: Needs review » Needs work

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

dutchyoda’s picture

Assigned: Unassigned » dutchyoda
dutchyoda’s picture

Assigned: dutchyoda » Unassigned
piyuesh23’s picture

Status: Needs work » Needs review
FileSize
186.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.

piyuesh23’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
4.73 KB

reroll!

Status: Needs review » Needs work

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

piyuesh23’s picture

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

piyuesh23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

piyuesh23’s picture

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.

piyuesh23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

piyuesh23’s picture

Renaming interdiff.patch to interdiff.txt.

piyuesh23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

piyuesh23’s picture

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

piyuesh23’s picture

Status: Needs work » Needs review
rahul.shinde’s picture

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

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

YesCT’s picture

Issue tags: +RTBC July 1

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

alexpott’s picture

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

piyuesh23’s picture

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

piyuesh23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

tim.plunkett’s picture

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

piyuesh23’s picture

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

piyuesh23’s picture

FileSize
1.51 KB

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

piyuesh23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

tim.plunkett’s picture

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

piyuesh23’s picture

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

piyuesh23’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This looks fine.

alexpott’s picture

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.

Gábor Hojtsy’s picture

Issue tags: +language-ui
cilefen’s picture