Files: 
CommentFileSizeAuthor
#53 1992606-53.patch39.75 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 57,894 pass(es).
[ View ]
#53 1992606-46-53-interdiff.txt249 bytespwieck
#47 1992606-46.patch39.74 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,641 pass(es).
[ View ]
#44 1992606-44.patch39.69 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 56,155 pass(es), 1 fail(s), and 5 exception(s).
[ View ]
#43 1992606-43.patch39.61 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,910 pass(es).
[ View ]
#40 1992606-40.patch39.7 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,378 pass(es).
[ View ]
#40 interdiff-1992606-40.txt4 KBdamiankloip
#37 1992606-37.patch39.2 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,963 pass(es).
[ View ]
#37 interdiff-1992606-37.txt766 bytesdamiankloip
#32 1992606-32.patch38.45 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,885 pass(es), 44 fail(s), and 4 exception(s).
[ View ]
#32 interdiff-1992606-32.txt4.16 KBdamiankloip
#28 1992606-28.patch33.48 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 50,700 pass(es), 2,506 fail(s), and 929 exception(s).
[ View ]
#28 interdiff-1992606-28.txt1.04 KBdamiankloip
#23 1992606-23.patch33.98 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 30,842 pass(es), 15,839 fail(s), and 1,543 exception(s).
[ View ]
#23 interdiff-1992606-23.txt3.21 KBdamiankloip
#17 1992606-17.patch37.96 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,186 pass(es).
[ View ]
#17 interdiff-1992606-17.txt1.86 KBdamiankloip
#14 1992606-14.patch36.62 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,641 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#14 interdiff-1992606-14.txt1.77 KBdamiankloip
#11 1992606-11.patch36.84 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,687 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#11 interdiff-1992606-11.txt1.25 KBdamiankloip
#9 1992606-9.patch35.6 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,470 pass(es), 60 fail(s), and 4 exception(s).
[ View ]
#5 1992606-5.patch35.55 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1992606-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#5 interdiff-1992606-5.txt3.27 KBdamiankloip
#3 1992606-3.patch35.2 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 56,253 pass(es), 165 fail(s), and 20 exception(s).
[ View ]
#1 1992606.patch30.17 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.php.
[ View ]

Comments

StatusFileSize
new30.17 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.php.
[ View ]

Here is an initial conversion of the class. I haven't touched hook menu yet, as it iterates through each theme to provide a path/route. Are there any examples in core already of how we plan to implement this with routes?

Assigned:Unassigned» damiankloip

Status:Active» Needs review
StatusFileSize
new35.2 KB
FAILED: [[SimpleTest]]: [MySQL] 56,253 pass(es), 165 fail(s), and 20 exception(s).
[ View ]

ok, added a RouteSubscriber and friends.

Status:Needs review» Needs work

The last submitted patch, 1992606-3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.27 KB
new35.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1992606-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This should improve things a bit.

Status:Needs review» Needs work

The last submitted patch, 1992606-5.patch, failed testing.

Status:Needs work» Needs review

#5: 1992606-5.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1992606-5.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new35.6 KB
FAILED: [[SimpleTest]]: [MySQL] 55,470 pass(es), 60 fail(s), and 4 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1992606-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.25 KB
new36.84 KB
FAILED: [[SimpleTest]]: [MySQL] 55,687 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

So the fails are because bartik is enabled, but caches need to be flushed to see the changes in the test env.

+++ b/core/modules/system/lib/Drupal/system/EventSubscriber/RouteSubscriber.phpundefined
@@ -0,0 +1,47 @@
+          '_permission' => 'administer themes',
+++ b/core/modules/system/system.moduleundefined
@@ -726,28 +726,25 @@ function system_menu() {
-      'access callback' => '_system_themes_access',
-      'access arguments' => array($key),
...
+    if (!empty($theme->status)) {

This doesn't seem right

+++ b/core/modules/system/lib/Drupal/system/EventSubscriber/RouteSubscriber.phpundefined
@@ -0,0 +1,47 @@
+class RouteSubscriber implements EventSubscriberInterface  {

double space before {

+++ b/core/modules/system/lib/Drupal/system/EventSubscriber/RouteSubscriber.phpundefined
@@ -0,0 +1,47 @@
+   * Implements EventSubscriberInterface::getSubscribedEvents().
+++ b/core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.phpundefined
@@ -0,0 +1,448 @@
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Overrides \Drupal\system\SystemConfigFormBase::buildForm().

inheritdoc

+++ b/core/modules/system/system.moduleundefined
@@ -726,28 +726,25 @@ function system_menu() {
+    'route_name' => 'system_theme_settings_global',

Regardless of the continuing discussion of what to do with MENU_DEFAULT_LOCAL_TASK, this route_name shouldn't be here.

Status:Needs review» Needs work

The last submitted patch, 1992606-11.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.77 KB
new36.62 KB
FAILED: [[SimpleTest]]: [MySQL] 55,641 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Thanks for the review.

This doesn't seem right

I have just used _permission in the route for the access because the other part of _system_theme_access (system_theme_access) just checks the status of the theme, which we can easily roll into the controller callback. This seems better than having an access checker just for this? Also, only enabled themes will get routes registered, and shouldn't rely on access being denied to hide them.

Fixed t'other things.

If not, let me know, and I can add an _access_system_theme access checker or something.

Status:Needs review» Needs work

The last submitted patch, 1992606-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
new37.96 KB
PASSED: [[SimpleTest]]: [MySQL] 56,186 pass(es).
[ View ]

Need to rebuild the router as well are call menu_router_rebuild().

Status:Needs review» Needs work

The last submitted patch, 1992606-17.patch, failed testing.

Status:Needs work» Needs review

#17: 1992606-17.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1992606-17.patch, failed testing.

Status:Needs work» Needs review

#17: 1992606-17.patch queued for re-testing.

Status:Needs review» Needs work

+++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.phpundefined
@@ -48,6 +48,8 @@ function setUp() {
+    \Drupal::service('router.builder')->rebuild();

Just use $this->container->get('router_builder')

+++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.phpundefined
@@ -48,6 +48,8 @@ function setUp() {
+    menu_router_rebuild();

What is the reason we have to rebuild now and we haven't needed it before?

+++ b/core/modules/system/lib/Drupal/system/EventSubscriber/RouteSubscriber.phpundefined
+++ b/core/modules/system/lib/Drupal/system/EventSubscriber/RouteSubscriber.phpundefined
@@ -0,0 +1,47 @@
@@ -0,0 +1,47 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\system\EventSubscriber\RouteSubscriber.
+ */
+
+namespace Drupal\system\EventSubscriber;
+
+use Drupal\Core\Routing\RouteBuildEvent;
+use Drupal\Core\Routing\RoutingEvents;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
...
+
+/**
+ * Event subscriber for routes.
+ */
+class RouteSubscriber implements EventSubscriberInterface {
+
+  /**
+   * Implements EventSubscriberInterface::getSubscribedEvents().
+   */
+  static function getSubscribedEvents() {
+    $events[RoutingEvents::DYNAMIC] = 'createSystemThemeRoutes';
+    return $events;
+  }
+
+  /**
+   * Adds dynamic system theme routes.
+   *
+   * @param \Drupal\Core\Routing\RouteBuildEvent $event
+   *   The route building event.
+   */
+  public function createSystemThemeRoutes(RouteBuildEvent $event) {
+    $collection = $event->getRouteCollection();
+    foreach (list_themes() as $theme) {
+      if (!empty($theme->status)) {
+        $route = new Route('admin/appearance/settings/' . $theme->name, array(
+          '_form' => '\Drupal\system\Form\ThemeSettingsForm', 'theme_name' => $theme->name), array(
+          '_permission' => 'administer themes',
+        ));
+        $collection->add('system_theme_settings_' . $theme->name, $route);
+      }
+    }
+  }
+

As talked there is no reason to just create a single route with a parameter.

+++ b/core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.phpundefined
@@ -0,0 +1,448 @@
+   * @var \Drupal\Core\Extension\ModuleHandler
...
+  public function __construct(ConfigFactory $config_factory, ContextInterface $context, ModuleHandler $module_handler) {

ModuleHandlerInterface should be used here

Assigned:damiankloip» Unassigned
Status:Needs work» Needs review
StatusFileSize
new3.21 KB
new33.98 KB
FAILED: [[SimpleTest]]: [MySQL] 30,842 pass(es), 15,839 fail(s), and 1,543 exception(s).
[ View ]

Ok, let's remove that route subscriber - seems to be pretty obsolete now.

I think we need to have a discussion with some folks about guidelines for access checkers.

Forgot to make the ModuleHandlerInterface change. I'll see how the test gets on then change it.

Status:Needs review» Needs work

The last submitted patch, 1992606-23.patch, failed testing.

Status:Needs work» Needs review

#23: 1992606-23.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1992606-23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
new33.48 KB
FAILED: [[SimpleTest]]: [MySQL] 50,700 pass(es), 2,506 fail(s), and 929 exception(s).
[ View ]

Hopefully this works, I'm bored of this issue.

Status:Needs review» Needs work

The last submitted patch, 1992606-28.patch, failed testing.

Status:Needs work» Needs review

#28: 1992606-28.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1992606-28.patch, failed testing.

StatusFileSize
new4.16 KB
new38.45 KB
FAILED: [[SimpleTest]]: [MySQL] 55,885 pass(es), 44 fail(s), and 4 exception(s).
[ View ]

Ok, so having one route for all the dynamic paths just doesn't work. It will blow up as multiple menu items will have the same route name. I took some inspiration from field UI, and they are using a RouteSubscriber, as I was before.

Here is a new patch with the feedback from #22 but based on the patch in #17, when I had the RouteSubscriber.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1992606-32.patch, failed testing.

Status:Needs work» Needs review

#32: 1992606-32.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1992606-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new766 bytes
new39.2 KB
PASSED: [[SimpleTest]]: [MySQL] 56,963 pass(es).
[ View ]

The build args have changed, so the form alter in color.module needs to reflect that.

+++ b/core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.phpundefined
@@ -0,0 +1,448 @@
+   * @var \Drupal\Core\Extension\ModuleHandler

Let's document the interface.

+++ b/core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.phpundefined
@@ -0,0 +1,448 @@
+  public function __construct(ConfigFactory $config_factory, ContextInterface $context, ModuleHandlerInterface $module_handler) {
...
+    $this->moduleHandler = $module_handler;

It would be cool to document the ModuleHandler here as well.

+++ b/core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.phpundefined
@@ -0,0 +1,448 @@
+    $toggles = array(
+      'logo'                      => t('Logo'),
+      'name'                      => t('Site name'),
+      'slogan'                    => t('Site slogan'),
+      'node_user_picture'         => t('User pictures in posts'),
+      'comment_user_picture'      => t('User pictures in comments'),
+      'comment_user_verification' => t('User verification status in comments'),
+      'favicon'                   => t('Shortcut icon'),
+      'main_menu'                 => t('Main menu'),
+      'secondary_menu'            => t('Secondary menu'),
+    );

While you fix some things anyway: please remove this ugly whitespace.

Status:Needs review» Needs work

Please revert that last interdiff and include this change instead:

diff --git a/core/lib/Drupal/Core/Controller/HtmlFormController.php b/core/lib/Drupal/Core/Controller/HtmlFormController.php
index 96530d5..68b6c31 100644
--- a/core/lib/Drupal/Core/Controller/HtmlFormController.php
+++ b/core/lib/Drupal/Core/Controller/HtmlFormController.php
@@ -57,10 +57,12 @@ public function content(Request $request, $_form) {
     $request->attributes->set('form', array());
     $request->attributes->set('form_state', $form_state);
     $args = $this->container->get('controller_resolver')->getArguments($request, array($form_object, 'buildForm'));
-    unset($args[0], $args[1]);
     $request->attributes->remove('form');
     $request->attributes->remove('form_state');
-    $form_state['build_info']['args'] = $args;
+
+    // Remove $form and $form_state from the arguments, and re-index them.
+    unset($args[0], $args[1]);
+    $form_state['build_info']['args'] = array_values($args);
     $form_id = _drupal_form_id($form_object, $form_state);
     $form = drupal_build_form($form_id, $form_state);

Status:Needs work» Needs review
StatusFileSize
new4 KB
new39.7 KB
PASSED: [[SimpleTest]]: [MySQL] 55,378 pass(es).
[ View ]

Thanks! All those changes make sense.

Status:Needs review» Reviewed & tested by the community

Thanks, this looks great now.

Status:Reviewed & tested by the community» Needs work

Needs a reroll

curl https://drupal.org/files/1992606-40.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 40650  100 40650    0     0  26990      0  0:00:01  0:00:01 --:--:-- 30842
error: patch failed: core/lib/Drupal/Core/Controller/HtmlFormController.php:57
error: core/lib/Drupal/Core/Controller/HtmlFormController.php: patch does not apply
error: patch failed: core/modules/system/system.routing.yml:122
error: core/modules/system/system.routing.yml: patch does not apply
error: patch failed: core/modules/system/system.services.yml:6
error: core/modules/system/system.services.yml: patch does not apply

Status:Needs work» Needs review
StatusFileSize
new39.61 KB
PASSED: [[SimpleTest]]: [MySQL] 55,910 pass(es).
[ View ]

*sigh* ...

StatusFileSize
new39.69 KB
FAILED: [[SimpleTest]]: [MySQL] 56,155 pass(es), 1 fail(s), and 5 exception(s).
[ View ]

And again.

Status:Needs review» Needs work

The last submitted patch, 1992606-44.patch, failed testing.

Assigned:Unassigned» damiankloip

Ah, I see what's wrong here.

Assigned:damiankloip» Unassigned
Status:Needs work» Needs review
StatusFileSize
new39.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,641 pass(es).
[ View ]

Needed a proper reroll after the File entity stuff.

Status:Needs review» Needs work

The last submitted patch, 1992606-46.patch, failed testing.

Status:Needs work» Needs review

#47: 1992606-46.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Looks good, thanks.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

curl https://drupal.org/files/1992606-46.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 40697  100 40697    0     0  20331      0  0:00:02  0:00:02 --:--:-- 22659
error: patch failed: core/modules/system/system.services.yml:9
error: core/modules/system/system.services.yml: patch does not apply

Assigned:Unassigned» pwieck

Working on re-roll now.

Status:Needs work» Needs review
StatusFileSize
new249 bytes
new39.75 KB
PASSED: [[SimpleTest]]: [MySQL] 57,894 pass(es).
[ View ]

re-rolled.

Assigned:pwieck» Unassigned
Issue tags:-Needs reroll

removed tag

Status:Needs review» Reviewed & tested by the community

Thanks for the reroll!

Status:Reviewed & tested by the community» Fixed

Committed a93de78 and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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