Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett’s picture

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs review » Needs work
Issue tags: +SprintWeekend2013
ACF’s picture

Status: Needs work » Needs review
FileSize
61.35 KB

Updated.

Status: Needs review » Needs work

The last submitted patch, 1925660-forminterface-system-drupal-4.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
FileSize
61.38 KB

Updated for 'route_name'.

Status: Needs review » Needs work
Issue tags: -FormInterface, -SprintWeekend2013

The last submitted patch, 1925660-forminterface-system-drupal-6.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review

#6: 1925660-forminterface-system-drupal-6.patch queued for re-testing.

Submitting for re-test as the repo failed to checkout from git.

Status: Needs review » Needs work
Issue tags: +FormInterface, +SprintWeekend2013

The last submitted patch, 1925660-forminterface-system-drupal-6.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
FileSize
61.38 KB

Updated, think I found the error.

Status: Needs review » Needs work
Issue tags: -FormInterface, -SprintWeekend2013

The last submitted patch, 1925660-forminterface-system-drupal-10.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review

#10: 1925660-forminterface-system-drupal-10.patch queued for re-testing.

Have gone for a retest because I can't replicate this issue locally. Although the three fails did relate to the admin page tests, which would definitely make sense.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +FormInterface, +SprintWeekend2013

The last submitted patch, 1925660-forminterface-system-drupal-10.patch, failed testing.

mtift’s picture

Assigned: Unassigned » mtift

Working on a re-roll

mtift’s picture

Status: Needs work » Needs review
FileSize
55.93 KB

My first attempt at a re-roll using git merge

star-szr’s picture

Status: Needs review » Needs work
FileSize
5.52 KB

Thanks @mtift! It looks like there is a hunk missing from the reroll, just a deletion (see attached). The difference in patch file size was my tip-off.

I did a git blame on system.admin.inc and saw that the conflict came from #1926228: Performance page provides incorrect/incomplete information about CSS and JS compression (first patch), so I think that committed string change should be made in this patch as well.

ACF’s picture

Status: Needs work » Needs review
FileSize
62.4 KB

re-rolled

Status: Needs review » Needs work

The last submitted patch, 1925660-forminterface-system-drupal-18.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
FileSize
62.33 KB

Oops, think I spotted the mistake.

Status: Needs review » Needs work

The last submitted patch, 1925660-forminterface-system-drupal-20.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
FileSize
62.33 KB

Should pass now.

mtift’s picture

Assigned: mtift » Unassigned
FileSize
62.33 KB
+++ b/core/modules/system/lib/Drupal/system/Form/CronForm.phpundefined
@@ -0,0 +1,125 @@
+use Drupal\Core\KeyValueStore\KeyValueFactory;
+use Symfony\Component\HttpFoundation\RedirectResponse;
+use Drupal\Core\Config\ConfigFactory;
+use Drupal\Core\Config\Context\ContextInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;

This is probably too nit picky, especially since #1624564: Coding standards for "use" statements (and elsewhere) discussed ordering "use statements" without coming to a resolution. That said, it seems like we could at least keep the Drupal and Symfony statements together, which is the only change in the attached patch.

The rest looks good to me, but I don't feel qualified to RTBC these.

tim.plunkett’s picture

Status: Needs review » Needs work

Close, some of this is just out of date due to other improvements.

+++ b/core/modules/system/lib/Drupal/system/Form/CronForm.phpundefined
@@ -0,0 +1,125 @@
+    $this->configFactory = $config_factory;
+    $this->configFactory->enterContext($context);

This should call parent::__construct()

+++ b/core/modules/system/lib/Drupal/system/Form/PerformanceForm.phpundefined
@@ -0,0 +1,142 @@
+    $config->set('cache.page.use_internal', $form_state['values']['cache']);
+    $config->set('cache.page.max_age', $form_state['values']['page_cache_maximum_age']);
+    $config->set('response.gzip', $form_state['values']['page_compression']);
+    $config->set('css.preprocess', $form_state['values']['preprocess_css']);
+    $config->set('js.preprocess', $form_state['values']['preprocess_js']);
+    $config->save();

This should use the chained approach, you don't need the $config variable.

+++ b/core/modules/system/lib/Drupal/system/Form/PerformanceForm.phpundefined
@@ -0,0 +1,142 @@
+    cache('page')->deleteAll();

This should have the cache() injected, or at least \Drupal::cache().

+++ b/core/modules/system/lib/Drupal/system/Form/SystemFormBase.phpundefined
@@ -0,0 +1,43 @@
+abstract class SystemFormBase extends SystemConfigFormBase implements ControllerInterface {
+
+  /**
+   * Constructs a \Drupal\language\Form\LanguageFormBase object.
+   *
+   * @param \Drupal\Core\Config\ConfigFactory $config_factory
+   *   The factory for configuration objects.
+   * @param \Drupal\Core\Config\Context\ContextInterface $context
+   *   The configuration context used for this configuration object.
+   */
+  public function __construct(ConfigFactory $config_factory, ContextInterface $context) {
+    $this->configFactory = $config_factory;
+    $this->configFactory->enterContext($context);
+  }
+
+  /**
+   * Implements \Drupal\Core\ControllerInterface::create().
+   */
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('config.factory'),
+      $container->get('config.context.free')
+    );
+  }
+
+}
diff --git a/core/modules/system/system.admin.inc b/core/modules/system/system.admin.inc

This can all go away, this was moved into SystemConfigFormBase directly

+++ b/core/modules/system/system.moduleundefined
@@ -802,11 +802,9 @@ function system_menu() {
     'access arguments' => array('administer site configuration'),

Here, and all of the rest, don't need access arguments anymore

heddn’s picture

heddn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1925660-forminterface-system-drupal-25.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
62.78 KB
2.79 KB

It would appear that the date configuration isn't included in the conversion effort on this issue. Let's try testing things again.

Status: Needs review » Needs work

The last submitted patch, 1925660-forminterface-system-drupal-28.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Adding system_image_toolkit_settings since it was added in #1664844: Convert image toolkits into plugins

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
16.56 KB
67.5 KB

The last config form in system.module is taken care of by #1992606: Convert system_theme_settings to FormInterface

ParisLiakos’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Form/CronForm.phpundefined
@@ -0,0 +1,126 @@
+  protected $store;
...
+    $this->state = $key_value_factory->get('state');

so is it state not store:)

+++ b/core/modules/system/lib/Drupal/system/Form/CronForm.phpundefined
@@ -0,0 +1,126 @@
+      '#submit' => array(array($this, 'submitCron')),

+++ b/core/modules/system/lib/Drupal/system/Form/PerformanceForm.phpundefined
@@ -0,0 +1,145 @@
+      '#submit' => array(array($this, 'submitCacheClear')),
...
+    $form['#submit'][] = array('systemClearPageCacheSubmit');
+    $form['#submit'][] = array('submitForm');

oh, does this finally work? am i outdated?:P

+++ b/core/modules/system/lib/Drupal/system/Form/PerformanceForm.phpundefined
@@ -0,0 +1,145 @@
+    drupal_add_library('system', 'drupal.system');

i wonder if we could use #attached here on the $form instead

+++ b/core/modules/system/lib/Drupal/system/Form/PerformanceForm.phpundefined
@@ -0,0 +1,145 @@
+    \Drupal::cache('page')->deleteAll();

cant we inject this?

+++ b/core/modules/system/lib/Drupal/system/Form/SiteInformationForm.phpundefined
@@ -0,0 +1,147 @@
+      form_set_value($form['front_page']['site_frontpage'], drupal_container()->get('path.alias_manager')->getSystemPath($form_state['values']['site_frontpage']), $form_state);
...
+      form_set_value($form['error_page']['site_403'], drupal_container()->get('path.alias_manager')->getSystemPath($form_state['values']['site_403']), $form_state);
...
+      form_set_value($form['error_page']['site_404'], drupal_container()->get('path.alias_manager')->getSystemPath($form_state['values']['site_404']), $form_state);

inject path alias manager?

+++ b/core/modules/system/system.moduleundefined
@@ -1023,20 +996,17 @@ function system_menu() {
-    'access arguments' => array('administer site configuration'),
...
   $items['admin/reports/status/run-cron'] = array(
...
-    'access arguments' => array('administer site configuration'),
...
   $items['admin/reports/status/php'] = array(
...
-    'access arguments' => array('administer site configuration'),

i cant see why those are removed

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
13.91 KB
68.04 KB

Thanks @ParisLiakos!

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

looks awesome now
also manually tested, and everything looks good
thanks!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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