Convert this page callback to a new-style FormInterface implementation, using the instructions on http://drupal.org/node/1800686.

Files: 
CommentFileSizeAuthor
#40 drupal-config_form-1945398-38.patch19.21 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 56,412 pass(es), 14 fail(s), and 8 exception(s).
[ View ]
#36 1945398-config-form-36.patch12.79 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 56,043 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#32 config-form-1945398-32.patch12.35 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-form-1945398-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#32 config-form-1945398-32-combined.patch69.97 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-form-1945398-32-combined.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 config-admin-1945398-29.patch11.71 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-admin-1945398-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 interdiff.txt7.93 KBtim.plunkett
#23 1945398.config-import-form.23.patch6.87 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 54,223 pass(es).
[ View ]
#23 interdiff.txt721 bytesmtift
#18 1945398.config-import-form.18.patch6.8 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 53,652 pass(es), 13 fail(s), and 5 exception(s).
[ View ]
#18 interdiff.txt6.08 KBmtift
#14 1945398.config-import-form.14.patch11.84 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 53,467 pass(es).
[ View ]
#14 interdiff.txt2.81 KBmtift
#11 9-11-interdiff.txt2.18 KBalexpott
#11 1945398.config-import-form.11.patch11.56 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 53,316 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#9 drupal-config_admin_import-1945398-9.patch11.61 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 53,291 pass(es), 13 fail(s), and 17 exception(s).
[ View ]
#9 interdiff.txt2.92 KBmtift
#6 drupal-config_admin_import-1945398-6.patch10.59 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 53,250 pass(es).
[ View ]

Comments

Tagging.

Assigned:Unassigned» mtift

I'll give this one a try

It looks like this issue might conflict with #1938338: Replace drupal_container() in Configuration system, but I think I can work on this issue anyway because config_menu() (in core/modules/config/config.module) will no longer reference config.admin.inc:

<?php
--- a/core/modules/config/config.module
+++ b/core/modules/config/config.module
@@ -43,10 +43,7 @@ function config_menu() {
  
$items['admin/config/development/sync'] = array(
    
'title' => 'Synchronize configuration',
    
'description' => 'Synchronize configuration changes.',
-   
'page callback' => 'drupal_get_form',
-   
'page arguments' => array('config_admin_import_form'),
-   
'access arguments' => array('synchronize configuration'),
-   
'file' => 'config.admin.inc',
+   
'route_name' => 'config_admin_import',
?>

And because config.admin.inc (and all 4 functions within it) will be replaced by core/modules/config/lib/Drupal/config/ConfigAdminImportForm.php and core/modules/config/config.routing.yml.

If any of my assumptions are incorrect, please let me know.

Status:Active» Needs review

Status:Needs review» Active

No patch here, so "active" is still the correct status. :)

Issue summary:View changes

Use issue link syntax.

StatusFileSize
new10.59 KB
PASSED: [[SimpleTest]]: [MySQL] 53,250 pass(es).
[ View ]

I wasn't sure why config_admin_sync_form() needed to be a helper function, so I threw all of that code in buildForm().

Status:Active» Needs review

Status:Needs review» Needs work

We should be injecting the config storage services... something like this...

Borrowed from #1925660: Convert system's system_config_form() to SystemConfigFormBase: Convert system's system_config_form() to SystemConfigFormBase:

  /**
   * Constructs a \Drupal\config\Form\ConfigAdminImportForm object.
   *
   * @param \Drupal\Core\Config\StorageInterface $source_storage
   *   ...
   * @param \Drupal\Core\Config\StorageInterface $target_storage
   *   ...
   */
  public function __construct(StorageInterface $source_storage, StorageInterface $target_storage) {
    $this->sourceStorage = $source_storage;
    $this->targetStorage = $target_storage;
  }
  /**
   * Implements \Drupal\Core\ControllerInterface::create().
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('config.storage.staging'),
      $container->get('config.storage')
    );
  }

Then instead of Drupal::service() you can use $this->sourceStorage / $this->targetStorage

Status:Needs work» Needs review
StatusFileSize
new2.92 KB
new11.61 KB
FAILED: [[SimpleTest]]: [MySQL] 53,291 pass(es), 13 fail(s), and 17 exception(s).
[ View ]

Thanks for the review @alexpott. Something isn't working, but I'm not sure what the problem is. It seems like the problem is either with the constructor:

<?php
 
public function __construct(StorageInterface $source_storage, StorageInterface $target_storage) {
   
$this->sourceStorage = $source_storage;
   
$this->targetStorage = $target_storage;
  }
?>

Or the listAll() method:

<?php
  $source_storage
->listAll();
?>

Any idea what I'm doing wrong?

Status:Needs review» Needs work

The last submitted patch, drupal-config_admin_import-1945398-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.56 KB
FAILED: [[SimpleTest]]: [MySQL] 53,316 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
new2.18 KB

@mtift... we need to implement the ControllerInterface... so that Drupal\Core\HtmlFormController::content() will call the static create() function.

-class ConfigAdminImportForm implements FormInterface {
+class ConfigAdminImportForm implements FormInterface, ControllerInterface {

The patch attached does this... and makes a couple of code style changes... but there are still 4 fails in Drupal\config\Tests\ConfigImportUITest for you to track down :)

Status:Needs review» Needs work

The last submitted patch, 1945398.config-import-form.11.patch, failed testing.

Assigned:mtift» Unassigned

I need a break from this patch. I'm unsure why those tests are failing. It seems like the problem has something to do with prepareSiteNameUpdate() in core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php, but I have been unable to figure out what needs to change, so I suspect I'm missing something here.

StatusFileSize
new2.81 KB
new11.84 KB
PASSED: [[SimpleTest]]: [MySQL] 53,467 pass(es).
[ View ]

With excellent guidance from @alexpott, I'm back on the case -- and any errors in the attached patch are mine, and not his.

Status:Needs work» Needs review

I wasn't sure why config_admin_sync_form() needed to be a helper function, so I threw all of that code in buildForm().

That's backward. :-) If you can factor the code out to utility methods to increase readability or reduce the size/number of conditional blocks etc., do so. Don't inline.

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigAdminImportForm.php
@@ -0,0 +1,188 @@
+    $config_changes = config_sync_get_changes($this->sourceStorage, $this->targetStorage);

This should be coming from an injected service. I don't know the config system well enough to say which one, but if it doesn't exist yet we should make it. (Config has no excuse to not be cleanly injected objects at this point.)

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigAdminImportForm.php
@@ -0,0 +1,188 @@
+    else if (config_import()) {

This should also be coming from something injected.

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigAdminImportForm.php
@@ -0,0 +1,188 @@
+      drupal_flush_all_caches();

I believe the cache system is now injectable, so this can be injected, too.

For the injectable config import service see #1890784: Refactor configuration import and sync functions

#1890784: Refactor configuration import and sync functions will allow you to inject the config stuff. Currently you can't.

Status:Needs review» Needs work
StatusFileSize
new6.08 KB
new6.8 KB
FAILED: [[SimpleTest]]: [MySQL] 53,652 pass(es), 13 fail(s), and 5 exception(s).
[ View ]

I brought back config_admin_sync_form(). It looks like config_sync_get_changes() is going away with #1890784: Refactor configuration import and sync functions, but that config_import() is not. And I'm still trying to figure out which service replaces drupal_flush_all_caches().

Looking at drupal_flush_all_caches(), there is no individual service that replicates that.

Status:Needs work» Needs review

OK, let's see if this new patch (#18) passes.

Status:Needs review» Needs work

The last submitted patch, 1945398.config-import-form.18.patch, failed testing.

Hm. OK, I was confused as to which cache function we were talking about. Yeah, that doesn't have a service equivalent yet. Drat.

Assigned:Unassigned» mtift
Status:Needs work» Needs review
StatusFileSize
new721 bytes
new6.87 KB
PASSED: [[SimpleTest]]: [MySQL] 54,223 pass(es).
[ View ]

Feels weird using module_load_include() here, but I'm not sure how else to call config_admin_sync_form() in this situation.

I'd think that config_admin_sync_form() would need to be converted first, and this would extend it.

It seems like this patch could be good as is.

Because config_admin_sync_form() calls config_sync_get_changes(), and because there are 7 calls to config_sync_get_changes(), and so forth, it seems like this could get complicated rather quickly. What does it mean to convert config_admin_sync_form()?

Status:Needs review» Needs work

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigAdminImportForm.phpundefined
@@ -0,0 +1,122 @@
+   * Implements \Drupal\Core\ControllerInterface::create().
...
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().

We have a new standard now, those should be {@inheritdoc}

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigAdminImportForm.phpundefined
@@ -0,0 +1,122 @@
+    module_load_include('inc', 'config', 'config.admin');

inject module_handler for that;)

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigAdminImportForm.php
@@ -0,0 +1,122 @@
+
+    module_load_include('inc', 'config', 'config.admin');
+    config_admin_sync_form($form, $form_state, $this->sourceStorage, $this->targetStorage);
+    return $form;

This is definitely a no-go. That function needs to be turned into a service, or form object, or whatever it is. :-)

Assigned:mtift» Unassigned

Too swamped to work on this right now

Status:Needs work» Needs review
StatusFileSize
new7.93 KB
new11.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-admin-1945398-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

config_sync_get_changes() will be handled in #1890784: Refactor configuration import and sync functions, no need to wait on that
config_import() needs its own issue (that would be blocked on that issue anyway, or an expanded part of it)

I talked to heyrocker, the config_admin_sync_form() was split out to be reusable for another form that never came to exist.
But now that it's a class, anyone who would have used that function can just extend this class.

Thanks again to @mtift, this was so close, sorry it got bogged down.

Status:Needs review» Needs work

The last submitted patch, config-admin-1945398-29.patch, failed testing.

Assigned:Unassigned» tim.plunkett
Status:Needs work» Postponed

Postponing on #1890784: Refactor configuration import and sync functions, it's getting very close.

Status:Postponed» Needs review
StatusFileSize
new69.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-form-1945398-32-combined.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new12.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-form-1945398-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled anyway. Contains the other. This thing is fully injected now!

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

The last submitted patch, config-form-1945398-32-combined.patch, failed testing.

Issue tags:+Needs reroll

Tagging

Status:Needs work» Needs review
StatusFileSize
new12.79 KB
FAILED: [[SimpleTest]]: [MySQL] 56,043 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

Re-rolling...

Status:Needs review» Needs work

The last submitted patch, 1945398-config-form-36.patch, failed testing.

Assigned:tim.plunkett» Unassigned

This doesn't apply anymore.

Status:Needs work» Needs review
StatusFileSize
new19.21 KB
FAILED: [[SimpleTest]]: [MySQL] 56,412 pass(es), 14 fail(s), and 8 exception(s).
[ View ]

As was stated above, this logic was almost completely redone in #1998576: Make the config import process use full config trees again. I've rerolled this and duplicated that functionality in in the Form Controller, but it is in need of refactoring. Of most importance, injecting services instead of calling them directly. It also might make sense to store the form object in the class itself instead of passing by reference to the helper function. I doubt I'll have time to get back to this in the next two weeks, so if someone else wants to take up that task, go for it.

Status:Needs review» Needs work

The last submitted patch, drupal-config_form-1945398-38.patch, failed testing.

Status:Needs work» Closed (duplicate)

Issue summary:View changes

Updated issue summary.