Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Status: Active » Needs review
FileSize
13.06 KB

First pass

marcingy’s picture

Title: Convert config_admin_diff_page() to a new style controller » Convert config_sync() to a new style controller

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, config-sync.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion

#1: config-sync.patch queued for re-testing.

aspilicious’s picture

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.phpundefined
@@ -0,0 +1,218 @@
+  ¶

Trailing whitespace

alexpott’s picture

Status: Needs review » Postponed

Postponing this on #1890784: Refactor configuration import and sync functions as that issue will result in significant changes to this patch

vijaycs85’s picture

Status: Postponed » Needs review
vijaycs85’s picture

Issue tags: -WSCCI-conversion

#1: config-sync.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, config-sync.patch, failed testing.

swiftsure’s picture

Assigned: Unassigned » swiftsure

Code sprint note.

I am working on this issue.

dougvann’s picture

Assigned: swiftsure » dougvann
Status: Needs work » Needs review
FileSize
7.38 KB

Rerolled patch
corrected white space in lib/Drupal/config/Form/ConfigSync.php
corrected file removal of config.admin.inc

BIG THNX to Benjifisher http://drupal.org/user/683300 for lots of help and my team mate swiftsure for our 1st major contribution.. ;-)

aspilicious’s picture

Status: Needs review » Needs work

The new file is missing :)
Ask for help on creating patches with new files!

Go for it!

dougvann’s picture

Status: Needs work » Needs review
FileSize
14 KB

Thnx aspilicious !
Here it is.

Status: Needs review » Needs work

The last submitted patch, config-sync-1987660-13.patch, failed testing.

dougvann’s picture

Status: Needs work » Needs review
FileSize
14 KB

My n00b mistake has been corrected. Addition of whitespace in hunk has been removed.

Status: Needs review » Needs work

The last submitted patch, config-sync-1987660-15.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
13.03 KB

Re-rolling...

Status: Needs review » Needs work

The last submitted patch, 1987660-config-sync-17.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
534 bytes
13.55 KB

Missed routing file change...

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, 1987660-config-sync-19.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

#19: 1987660-config-sync-19.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, 1987660-config-sync-19.patch, failed testing.

ygerasimov’s picture

Assigned: dougvann » Unassigned
Status: Needs work » Needs review
FileSize
7.29 KB
14.8 KB

Rebuilt the patch (nearly from scratch). Lets see how tests will pass.

dawehner’s picture

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.phpundefined
@@ -0,0 +1,246 @@
+    $this->config_admin_sync_form($form, $form_state);

Let's camelCase the method.

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.phpundefined
@@ -0,0 +1,246 @@
+  private function config_admin_sync_form(array &$form, array &$form_state) {

I don't see why we need a helper function here ...

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.phpundefined
@@ -0,0 +1,246 @@
+          'title' => t('View differences'),

t could be replaced by the translation manager.

star-szr’s picture

Status: Needs review » Needs work

Based on the above this needs work.

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
6.36 KB
15.02 KB

Patch changed according to #24. Thank you for the review.

dawehner’s picture

+++ b/core/modules/config/config.routing.ymlundefined
@@ -22,3 +22,9 @@ config_import:
+  pattern: 'admin/config/development/sync'

Pattern should start with a slash.

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.phpundefined
@@ -0,0 +1,246 @@
+   *
+   * @var \Drupal\Core\StringTranslation\TranslationManager
...
+   * @param \Drupal\Core\StringTranslation\TranslationManager
...
+  public function __construct(StorageInterface $sourceStorage, StorageInterface $targetStorage, LockBackendInterface $lock, EventDispatcherInterface $event_dispatcher, ConfigFactory $config_factory, EntityManager $entity_manger, TranslationManager $translation_manager) {

I would vote to go with the TranslatorInterface

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.phpundefined
@@ -0,0 +1,246 @@
+          'href' => 'admin/config/development/sync/diff/' . $config_file,

For now we could use getPathFromRoute on the url generator.

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.phpundefined
@@ -0,0 +1,246 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function validateForm(array &$form, array &$form_state) {
+  }

Let's keep the usual order and move validateForm before submitForm

ygerasimov’s picture

Rerolled patch.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Just in general this page is really nice with its diff functionality.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0adc91a and pushed to 8.x. Thanks!

-        // Return a negative result for UI purposes. We do not differentiate between
-        // an actual synchronization error and a failed lock, because concurrent
-        // synchronizations are an edge-case happening only when multiple developers
-        // or site builders attempt to do it without coordinating.
+        // Return a negative result for UI purposes. We do not differentiate
+        // between an actual synchronization error and a failed lock, because
+        // concurrent synchronizations are an edge-case happening only when
+        // multiple developers or site builders attempt to do it without
+        // coordinating.

Comment needed to be reformatted so that lines where less than 80 characters long.

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