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
#24 date-formats-1987820-24.patch10.33 KBstevector
Test request sent.
[ View ]
#24 interdiff24.txt2.49 KBstevector
#22 date-formats-1987820-22.patch10.56 KBstevector
PASSED: [[SimpleTest]]: [MySQL] 56,928 pass(es).
[ View ]
#22 interdiff.txt2.25 KBstevector
#20 drupal-1987820-20.patch9.52 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,114 pass(es).
[ View ]
#20 interdiff.txt3.45 KBdawehner
#18 1987820_17.patch6.07 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 57,058 pass(es), 7 fail(s), and 2 exception(s).
[ View ]
#15 drupal-1987820-14.patch7.14 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#15 interdiff.txt1.31 KBdawehner
#12 1987820-12.patch5.79 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 55,649 pass(es), 9 fail(s), and 2 exception(s).
[ View ]
#12 interdiff.txt2.58 KBjibran
#12 interdiff1.txt1.03 KBjibran
#9 1987820_9.patch3.67 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#8 1987820_8.patch3.7 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#6 1987820_6.patch3.73 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#2 1987820_2.patch3.81 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

I tried to assign this issue to myself. Going to take a shot at this.

StatusFileSize
new3.81 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here's a first stab at this:

Assigned:Unassigned» cosmicdreams

Status:Active» Needs work

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,83 @@
+/**
+ * Created by JetBrains PhpStorm.
+ * User: karen
+ * Date: 5/19/13
+ * Time: 11:21 AM
+ * To change this template use File | Settings | File Templates.
+ */

@file missing

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,83 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+

Not needed.

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,83 @@
\ No newline at end of file

newline missing.

+++ b/core/modules/system/system.routing.ymlundefined
@@ -87,3 +87,10 @@ date_format_localize_reset:
\ No newline at end of file

Same as above

Assigned:cosmicdreams» Unassigned
Status:Needs work» Active

In general you should also remove the old code.

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,83 @@
+ * Created by JetBrains PhpStorm.
+ * User: karen
+ * Date: 5/19/13
+ * Time: 11:21 AM
+ * To change this template use File | Settings | File Templates.

ups :)

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,83 @@
+use Drupal\Core\Config\ConfigFactory;
+use Drupal\Core\Controller\ControllerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;

None of this classes are actually used. Phpstorm shows you the code in grey if it's not used (note: plugin annotation aren't seen).

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,83 @@
+class DateTimeController {
...
+  public function formats() {

I'm sorry both these needs some docs.

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,83 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+

Wow that's a lot of space

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,83 @@
\ No newline at end of file
+++ b/core/modules/system/system.routing.ymlundefined
@@ -87,3 +87,10 @@ date_format_localize_reset:
\ No newline at end of file

Missing endline at the end of the file.

StatusFileSize
new3.73 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Cleaned up the comments as per above.

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,69 @@
\ No newline at end of file

newline still missing :)

StatusFileSize
new3.7 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

ok

StatusFileSize
new3.67 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

no here

Status:Active» Needs review

Let's test it.

Status:Needs review» Needs work

We have to remove system_date_time_formats in system.admin.inc.

StatusFileSize
new1.03 KB
new2.58 KB
new5.79 KB
FAILED: [[SimpleTest]]: [MySQL] 55,649 pass(es), 9 fail(s), and 2 exception(s).
[ View ]
  • Removed system_date_time_formats in system.admin.inc.
  • Fixed docs.

Status:Needs work» Needs review

For the test.

Status:Needs review» Needs work

The last submitted patch, 1987820-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
new7.14 KB
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

While manually testing the patch I realized that this would break adding a date format and get redirected to admin/config/regional/date-time/formats
which breaks with this change (due to the route system).

We can fix the tests by removing /formats, which is not a problem, as we don't have local tabs here.

Status:Needs review» Needs work

The last submitted patch, drupal-1987820-14.patch, failed testing.

This one might fix the two test fails.

Status:Needs work» Needs review
StatusFileSize
new6.07 KB
FAILED: [[SimpleTest]]: [MySQL] 57,058 pass(es), 7 fail(s), and 2 exception(s).
[ View ]

missing the patch, here it is

Status:Needs review» Needs work

The last submitted patch, 1987820_17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.45 KB
new9.52 KB
PASSED: [[SimpleTest]]: [MySQL] 56,114 pass(es).
[ View ]

Let's fix the failures.

I'm at a table at the Portland sprint right now. We're looking at this patch and scratching our heads over the

use Symfony\Component\DependencyInjection\ContainerInterface;

which is not actually used in the file. We suspect it will be advantageous to actually use ContainerInterface in the future. We'll grab Crell when he gets here.

StatusFileSize
new2.25 KB
new10.56 KB
PASSED: [[SimpleTest]]: [MySQL] 56,928 pass(es).
[ View ]

We got some feedback form Crell. Mainly I'm not sure if it makes sense to take out system_date_time_formats() yet as there could be a better answer that replaces that functionality everywhere. I think the that function and related functions need to be a service but it's likely that ideal should stay out of the way of the present conversion issue.

I do think "DateTimeController" is too general. Thoughts?

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,102 @@
+class DateTimeController implements ControllerInterface {

What about DateFormatsListing?

+++ b/core/modules/system/lib/Drupal/system/Controller/DateTimeController.phpundefined
@@ -0,0 +1,102 @@
+    // @todo, Crell suggests replacing the direct usage of system_get_date_formats()
+    // with this protected method that gets the config system out of the container
+    // and then does the same task as system_get_date_formats(). I'm not sure
+    // If that should be done now or in a bulk replacement of system_get_date_formats()

I would argue that this should be more like a service, because having a protected method on this object would make it impossible to reuse it. system_get_date_formats seems to be a function you might want to call from outside.

Issue tags:+Stalking Crell
StatusFileSize
new2.49 KB
new10.33 KB
Test request sent.
[ View ]

Is this a decent middle ground short of making date formats be a service themselves? This patch adds injection of the ConfigFactory. This issue is my first route conversion so I'm still feeling out the new guidelines.

Issue tags:-Stalking Crell

Larry said he'll see this just because it's a conversion issue. Doesn't need "Stalking Crell."

This actually duplicates the efforts of #2003892: Convert date formats to config entities
Can we mark this one as a duplicate?

Status:Needs review» Closed (duplicate)

Then let's do it.