Support from Acquia helps fund testing for Drupal Acquia logo

Comments

MattDanger’s picture

Assigned: Unassigned » MattDanger
MattDanger’s picture

Status: Active » Needs review
FileSize
4.07 KB

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion
MattDanger’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
pdrake’s picture

+++ b/core/modules/system/lib/Drupal/system/LanguageController.phpundefined

I believe this should be in a Controller directory. Otherwise, this looks pretty good to me.

MattDanger’s picture

Moved LanguageController.php to lib/Drupal/system/Controller

ParisLiakos’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Controller/LanguageController.phpundefined
@@ -0,0 +1,50 @@
+ * Definition of Drupal\system\Controller\LanguageController.

Should be Contains \Drupal\system\Controller\LanguageController

+++ b/core/modules/system/lib/Drupal/system/Controller/LanguageController.phpundefined
@@ -0,0 +1,50 @@
+   * @see locale_menu()

we remove those from the controllers

+++ b/core/modules/system/lib/Drupal/system/Controller/LanguageController.phpundefined
@@ -0,0 +1,50 @@
+  public function overviewPage() {

misses @return docs

+++ b/core/modules/system/lib/Drupal/system/Controller/LanguageController.phpundefined
@@ -0,0 +1,50 @@
+  ¶
...
+  ¶
...
+  ¶

unneeded whitespace

+++ b/core/modules/system/lib/Drupal/system/Controller/LanguageController.phpundefined
@@ -0,0 +1,50 @@
+    return theme('table', array('header' => $header, 'rows' => $rows));

this should return a render array

disasm’s picture

attached patch fixes comments in #7.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-convert_system_date_format_overview_page-1987818-8.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
590 bytes
4.14 KB

Syntax for render array was wrong. Looked it up, and should be good now. Ran failed test locally, and passed without issues.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

yeah its perfect now, thanks!

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
Issue tags: +Needs reroll

Needs a reroll

git ac https://drupal.org/files/drupal-convert_system_date_format_overview_page-1987818-11.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4243  100  4243    0     0   3665      0  0:00:01  0:00:01 --:--:--  4334
error: patch failed: core/modules/system/system.routing.yml:95
error: core/modules/system/system.routing.yml: patch does not apply
aks22’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll, -WSCCI-conversion, -RTBC July 1

Status: Needs review » Needs work
Issue tags: +Needs reroll, +WSCCI-conversion, +RTBC July 1

The last submitted patch, drupal-convert_system_date_format_overview_page-1987818-11.patch, failed testing.

jlbellido’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.25 KB

Rerolled #11 and removed tag

penyaskito’s picture

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +Needs reroll
jlbellido’s picture

I have reviewed my reroll #17 and it was wrong:
- It add code that in HEAD is deleted.
- It doesn't delete 'system_date_format_language_overview_page' function from system.admin.inc. This function is deleted in #11.

For fix this , i add a new reroll.

YesCT’s picture

Status: Needs work » Needs review

sending to the testbot by changing status to needs review.

Gaelan’s picture

Issue tags: -Needs reroll
vijaycs85’s picture

FileSize
34.03 KB

Tested patch in #19 and working as expected

Steps below:

1. Applied patch
2. Enabled language module
3. Added new language
4. Visited admin/config/regional/date-time/locale (I couldn't find a link in UI to go this path though)

Attaching screenshot of the page.

RTBC from my side.

Gábor Hojtsy’s picture

Issue tags: +language-base

Feel free to continue working on this, as per Dries' post this is still possible: http://buytaert.net/drupal-8-api-freeze (see Deprecating Drupal 7 APIs which according to @xjm applies to issues such as this one).

Fix tagging for D8MI.

+++ b/core/modules/system/lib/Drupal/system/Controller/LanguageController.phpundefined
@@ -0,0 +1,51 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\system\Controller\LanguageController.
+ */
+
+namespace Drupal\system\Controller;
+
+/**
+ * Controller for Language handling.
+ */
+class LanguageController {
+
+  /**
+   * Displays edit date format links for each language.
+   *
+   * @return array
+   *   Render array of overview page.
+   */

This is just for date formats. How does this become a "LanguageController"? Sounds very generic.

+++ b/core/modules/system/lib/Drupal/system/Controller/LanguageController.phpundefined
@@ -0,0 +1,51 @@
+      $links['edit'] = array(
+        'title' => t('Edit'),
+        'href' => "admin/config/regional/date-time/locale/$langcode/edit",
+      );
+      $links['reset'] = array(
+        'title' => t('Reset'),
+        'href' => "admin/config/regional/date-time/locale/$langcode/reset",
+      );

May be too much to change paths, but 'locale' is so-so-so wrong!

Also should use string concatenation, not double quotes.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Gábor Hojtsy’s picture

Issue tags: +API change, +API clean-up

Add API change tags.

jlbellido’s picture

Added the following updates:

- Renamed "LanguageController" class to "LanguageDateFormatController".
- Changed double quotes to string concatenated.
- Changed paths "admin/config/regional/date-time/locale/$langcode/reset" to "admin/config/regional/date-time/$langcode/reset"

This last change needs work because i think that parent path "admin/config/regional/date-time/locale" should be changed to "admin/config/regional/date-time/" too. This should do changes in Tests , ect.. Should it?

Gábor Hojtsy’s picture

Is this page still doing something useful? Languages are already assigned on the main editing page. What do the edit controllers do? Changing the path here is certainly not good in itself so long as the edit paths are nt reacting to it. I think we should figure out first and foremost if this is not just leftover code that should just be removed.

YesCT’s picture

Status: Needs work » Needs review

needs review just to let the bot have a go at testing it.
As #27 we still need to look and see if we need this at all.

Status: Needs review » Needs work

The last submitted patch, drupal-convert_system_date_format_overview_page-1987818-26.patch, failed testing.

vijaycs85’s picture

Re-rolling...

Reg #27, we have two pages related to language for dateFormat(see attached #31). Don't see any link for this locale page though.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
41.07 KB
76.56 KB

Attaching screenshots.

Gábor Hojtsy’s picture

What do these pages even do and how do they relate? #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed claims the combination of UIs is broken and regressed features from Drupal 7 unintentionally.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

It is a simple conversion.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Yeah, I agree it is best to just get this in for now, but the whole date localization UI / process / backend is a disaster in Drupal 8: #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks good.

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

All right, now let's move onto the heap of issues in #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed and fix the bugs.

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