Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Convert this page callback to a new-style FormInterface implementation, using the instructions on http://drupal.org/node/1800686.
Comment | File | Size | Author |
---|---|---|---|
#40 | drupal-config_form-1945398-38.patch | 19.21 KB | disasm |
#36 | 1945398-config-form-36.patch | 12.79 KB | vijaycs85 |
#32 | config-form-1945398-32.patch | 12.35 KB | tim.plunkett |
#32 | config-form-1945398-32-combined.patch | 69.97 KB | tim.plunkett |
#29 | config-admin-1945398-29.patch | 11.71 KB | tim.plunkett |
Comments
Comment #1
Alan Evans CreditAttribution: Alan Evans commentedTagging.
Comment #2
mtiftI'll give this one a try
Comment #3
mtiftIt 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:
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.
Comment #4
mtiftComment #5
xjmNo patch here, so "active" is still the correct status. :)
Comment #5.0
xjmUse issue link syntax.
Comment #6
mtiftI wasn't sure why config_admin_sync_form() needed to be a helper function, so I threw all of that code in buildForm().
Comment #7
mtiftComment #8
alexpottWe 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:
Then instead of
Drupal::service()
you can use$this->sourceStorage
/$this->targetStorage
Comment #9
mtiftThanks 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:
Or the listAll() method:
Any idea what I'm doing wrong?
Comment #11
alexpott@mtift... we need to implement the ControllerInterface... so that Drupal\Core\HtmlFormController::content() will call the static create() function.
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 :)Comment #13
mtiftI 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.
Comment #14
mtiftWith excellent guidance from @alexpott, I'm back on the case -- and any errors in the attached patch are mine, and not his.
Comment #15
Crell CreditAttribution: Crell commentedThat'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.
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.)
This should also be coming from something injected.
I believe the cache system is now injectable, so this can be injected, too.
Comment #16
alexpottFor the injectable config import service see #1890784: Refactor configuration import and sync functions
Comment #17
tim.plunkett#1890784: Refactor configuration import and sync functions will allow you to inject the config stuff. Currently you can't.
Comment #18
mtiftI 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().
Comment #19
tim.plunkettLooking at drupal_flush_all_caches(), there is no individual service that replicates that.
Comment #20
mtiftOK, let's see if this new patch (#18) passes.
Comment #22
Crell CreditAttribution: Crell commentedHm. OK, I was confused as to which cache function we were talking about. Yeah, that doesn't have a service equivalent yet. Drat.
Comment #23
mtiftFeels weird using
module_load_include()
here, but I'm not sure how else to callconfig_admin_sync_form()
in this situation.Comment #24
tim.plunkettI'd think that config_admin_sync_form() would need to be converted first, and this would extend it.
Comment #25
mtiftIt 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()?
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commentedWe have a new standard now, those should be {@inheritdoc}
inject module_handler for that;)
Comment #27
Crell CreditAttribution: Crell commentedThis is definitely a no-go. That function needs to be turned into a service, or form object, or whatever it is. :-)
Comment #28
mtiftToo swamped to work on this right now
Comment #29
tim.plunkettconfig_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.
Comment #31
tim.plunkettPostponing on #1890784: Refactor configuration import and sync functions, it's getting very close.
Comment #32
tim.plunkettRerolled anyway. Contains the other. This thing is fully injected now!
Comment #33
kim.pepper#32: config-form-1945398-32-combined.patch queued for re-testing.
Comment #35
kim.pepperTagging
Comment #36
vijaycs85Re-rolling...
Comment #38
tim.plunkettThis doesn't apply anymore.
Comment #39
kim.pepperI believe this was largely re-written with #1998576: Make the config import process use full config trees again
Comment #40
disasm CreditAttribution: disasm commentedAs 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.
Comment #42
tim.plunkettDupe of #1987660: Convert config_sync() to a new style controller
Comment #42.0
tim.plunkettUpdated issue summary.