Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alan Evans’s picture

Tagging.

mtift’s picture

Assigned: Unassigned » mtift

I'll give this one a try

mtift’s picture

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:

--- 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.

mtift’s picture

Status: Active » Needs review
xjm’s picture

Status: Needs review » Active

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

xjm’s picture

Issue summary: View changes

Use issue link syntax.

mtift’s picture

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

mtift’s picture

Status: Active » Needs review
alexpott’s picture

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

mtift’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
11.61 KB

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:

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

Or the listAll() method:

  $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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
11.56 KB
2.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.

mtift’s picture

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.

mtift’s picture

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

Crell’s picture

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.

alexpott’s picture

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

tim.plunkett’s picture

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

mtift’s picture

Status: Needs review » Needs work
FileSize
6.08 KB
6.8 KB

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().

tim.plunkett’s picture

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

mtift’s picture

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.

Crell’s picture

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

mtift’s picture

Assigned: Unassigned » mtift
Status: Needs work » Needs review
FileSize
721 bytes
6.87 KB

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

tim.plunkett’s picture

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

mtift’s picture

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()?

ParisLiakos’s picture

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;)

Crell’s picture

+++ 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. :-)

mtift’s picture

Assigned: mtift » Unassigned

Too swamped to work on this right now

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.93 KB
11.71 KB

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.

tim.plunkett’s picture

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

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

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
69.97 KB
12.35 KB

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

kim.pepper’s picture

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

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

kim.pepper’s picture

Issue tags: +Needs reroll

Tagging

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
12.79 KB

Re-rolling...

Status: Needs review » Needs work

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

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

This doesn't apply anymore.

kim.pepper’s picture

disasm’s picture

Status: Needs work » Needs review
FileSize
19.21 KB

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.

tim.plunkett’s picture

Status: Needs work » Closed (duplicate)
tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.