Updated: Comment #44

Problem/Motivation

Import process cannot handle modules being enabled/disabled. This issue was discussed at length in Prague. The UI with respect to synching configuration probably needs to special case enabling and disabling of modules. Anything that triggers data deletion needs to notify the user. This is related to #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall but we should remember that during config import that core only support importing a complete config tree.

Proposed resolution

Make the configuration import a 2-step process: (1) handle any module changes, then (2) do the current import process. Account for the various scenarious outlined in #31

Remaining tasks

  • (done) Waiting on #1969800: Add UUIDs to default configuration
  • (ongoing) open follow-up and "good idea" issues found while working on this issue
  • check to make sure all follow-ups and related issues were opened (remove the needs tag when that is done)

User interface changes

None

#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
#1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config
#1890784: Refactor configuration import and sync functions
#1918926: Module dependencies are not respected when default configuration is imported
#1969800: Add UUIDs to default configuration
#2029771: Having installation profiles in system.module.yml causes config import to fail
#2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler)
#2030073: Config cannot be imported in order for dependencies

Blockers of this issue

None.
(was blocking) #2030073: Config cannot be imported in order for dependencies

Follow-ups

Follow-ups needed because this was committed:

Spin off to separate issues

Good ideas of separate parts discussed or found during this issue, but did not blocking this, and might have been done even if this issue had not been fixed (not follow-ups)

Files: 
CommentFileSizeAuthor
#128 1808248.128.patch84.58 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,505 pass(es).
[ View ]
#128 125-128-interdiff.txt1.97 KBalexpott
#125 1808248.125.patch84.27 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,553 pass(es).
[ View ]
#125 123-125-interdiff.txt810 bytesalexpott
#125 progress.png23.73 KBalexpott
#123 1808248.123.patch84.19 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,457 pass(es).
[ View ]
#123 121-123-interdiff.txt737 bytesalexpott
#121 1808248.121.patch84.18 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,461 pass(es).
[ View ]
#121 118-121-interdiff.txt4.95 KBalexpott
#118 1808248.118.patch83.48 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,508 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#118 116-118-interdiff.txt1.41 KBalexpott
#116 1808248.116.patch81.96 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,419 pass(es).
[ View ]
#116 114-116-interdiff.txt12.77 KBalexpott
#114 1808248.114.patch77.18 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,447 pass(es).
[ View ]
#114 112-114-interdiff.txt11.54 KBalexpott
#112 1808248.112.patch76.89 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,489 pass(es).
[ View ]
#112 111-112-interdiff.txt18.89 KBalexpott
#111 1808248.111.patch75.63 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,425 pass(es).
[ View ]
#111 110-111-interdiff.txt6.16 KBalexpott
#110 1808248.110.patch76 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,452 pass(es).
[ View ]
#110 108-110-interdiff.txt21.23 KBalexpott
#108 1808248-108.patch74.9 KBbeejeebus
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,038 pass(es), 14 fail(s), and 5 exception(s).
[ View ]
#105 1808248.104.patch62.63 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,208 pass(es).
[ View ]
#105 102-104-interdiff.txt677 bytesalexpott
#102 1808248.102.patch61.81 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,153 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
#102 98-102-interdiff.txt3.34 KBalexpott
#102 1808248.102.patch61.81 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,157 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
#98 1808248.98.patch60.88 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,347 pass(es), 363 fail(s), and 17 exception(s).
[ View ]
#98 96-98-interdiff.txt7.96 KBalexpott
#96 1808248.95.patch51.46 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,952 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#96 93-95-interdiff.txt2.19 KBalexpott
#95 93-95-interdiff.txt2.19 KBalexpott
#93 1808248.93.patch50.8 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,161 pass(es), 31 fail(s), and 0 exception(s).
[ View ]
#93 91-93-interdiff.txt1.46 KBalexpott
#91 1808248.91.patch49.71 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#91 89-91-interdiff.txt407 bytesalexpott
#89 1808248.89.patch49.71 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/forum/forum.install.
[ View ]
#80 1808248-80.patch45.72 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 63,672 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#76 8409833.76.patch2.63 MBNitesh Sethia
FAILED: [[SimpleTest]]: [MySQL] 60,628 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#70 1808248.70.patch50.59 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 60,540 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#65 1808248-65.patch19.25 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 59,054 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#63 1808248-63.patch16.99 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 58,846 pass(es).
[ View ]
#61 1808248-61.patch16.99 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 58,842 pass(es).
[ View ]
#59 1808248-59.patch16.73 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 59,301 pass(es), 0 fail(s), and 16 exception(s).
[ View ]
#57 1808248-57.patch16.78 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/config/lib/Drupal/config/Form/ConfigSync.php.
[ View ]
#55 1808248-55.patch52.48 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 50,706 pass(es), 936 fail(s), and 2,092 exception(s).
[ View ]
#53 1808248-53.patch15.29 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 59,175 pass(es), 0 fail(s), and 30 exception(s).
[ View ]
#48 1808248-48.patch9.07 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 58,630 pass(es), 0 fail(s), and 692 exception(s).
[ View ]
#39 1808248-config-import-enable.patch236.49 KBtayzlor
FAILED: [[SimpleTest]]: [MySQL] 58,135 pass(es), 3 fail(s), and 625 exception(s).
[ View ]
#36 1808248-config-import-enable.patch230.06 KBtayzlor
FAILED: [[SimpleTest]]: [MySQL] 58,171 pass(es), 3 fail(s), and 624 exception(s).
[ View ]
#36 30-36-interdiff.txt222.08 KBtayzlor
#30 28-30-interdiff.txt523 bytesalexpott
#30 1808248.30.config-import-enable.test-only.patch17.64 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 57,153 pass(es).
[ View ]
#28 26-28-interdiff.txt2.42 KBalexpott
#28 1808248.28.config-import-enable.test-only.patch16.97 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 53,042 pass(es), 116 fail(s), and 70 exception(s).
[ View ]
#26 24-26-interdiff.txt4.93 KBalexpott
#26 1808248.26.config-import-enable.test-only.patch16.71 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#24 1808248.24.config-import-enable.test-only.patch17.08 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 55,718 pass(es), 0 fail(s), and 6,228 exception(s).
[ View ]
#18 1808248.18.condig-import-enable.test-only.patch3.62 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 49,270 pass(es), 5 fail(s), and 3 exception(s).
[ View ]
#15 1808248-15-config-import-modules.patch8.32 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 42,815 pass(es).
[ View ]
#9 1808248-9-config-import-modules.patch7.08 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 42,273 pass(es), 2 fail(s), and 6 exception(s).
[ View ]

Comments

beejeebus’s picture

Assigned:Unassigned» beejeebus

i have some code i can resurrect for this, if i don't post a patch by saturday, feel free to reassign.

beejeebus’s picture

Issue tags:+Configuration system

tagging.

andypost’s picture

Maybe better change title to - Allow configuration import to enable/disable modules :)

beejeebus’s picture

Title:add module support to the CMI import process» Allow configuration import to enable/disable modules
sun’s picture

Title:Allow configuration import to enable/disable modules» Changes in enabled modules are not properly handled by the config import process
Category:task» bug
Issue tags:+Needs tests, +Configurables

Yes, two passes sound sensible to me.

This shouldn't actually be very hard; I expect a ~10 lines patch here.

There's one complexity to this though:

1) Modules that need to be enabled need to be enabled before executing the import.

2) Modules that need to be disabled... are a different can of worms. Upfront or after? Will import hooks have to be executed or not? Dependencies?

3) Modules that need to be uninstalled... oh.my.holy.f/me runs.

And: Speaking of hooks in 1) and 2), I guess we actually need to run through module_enable() + module_disable().

Lastly, do we handle themes at the same time? Or separate issue? Or not at all? (I'd disagree with the last option.)

chx’s picture

This rabbit hole is very deep. I suspect that we need to define an explicit dependency order between CMI objects (and have some sensible defaults, very likely building on the module dependencies) and import in that order and perhaps reboot Drupal (batch API? Just a big bad reset? You need to reset statics and rebuild the DIC, at least) between steps. It's a gigantic mess.

Think of modules, field types, entity types, entity bundles, fields, instances, actions and what dependencies they can have on each other.

sun’s picture

Because it wasn't mentioned yet, it needs to be asked, since we briefly discussed the alternative option a while ago:

What if we'd apply a special behavior to extension handling in the config import process?

I.e., instead of enabling/disabling extensions the normal way, we just update the respective system lists, and if necessary, install the schema. That would exclude hook_install(), hook_enable(), hook_disable(), and installation of default config. We'd reset/reboot once after doing that, and then go ahead with the existing import process.

This alternative sounds scary to me, but mayhaps, it is less scary than the problem space of having to deal with interdependencies between extensions.

OTOH, dependencies in config should actually be off-topic here -- that's what #1605460: Implement dependencies and defer process for importing configuration is about. Dependencies between extensions are properly handled built-in already; that's what the second argument to module_enable() & co is for. Perhaps it makes most sense to focus and progress in a pragmatic way.

beejeebus’s picture

at this stage, i think all options should stay on the table. i'm almost finished a first cut naive-as-possible patch.

i definitely see it as a discussion starter - i'm ok with rejecting the approach i'm taking entirely if we can come up with something better.

beejeebus’s picture

Status:Active» Needs review
StatusFileSize
new7.08 KB
FAILED: [[SimpleTest]]: [MySQL] 42,273 pass(es), 2 fail(s), and 6 exception(s).
[ View ]

first cut attached with simple tests.

handles importing module enable, disable, unintsall and enable-disable cases.

i've taken a blindly optimistic and naive approach to this code, but it will hopefully give us something to shoot at in the form of tests for nastier scenarios that break this code.

beejeebus’s picture

Status:Needs work» Needs review
Issue tags:+Needs tests, +Configuration system, +Configurables

#9: 1808248-9-config-import-modules.patch queued for re-testing.

sun’s picture

Title:Changes in enabled modules are not properly handled by the config import process» Config import hooks of newly enabled extensions are not invoked in the config staging/import process

Sorry for the noise, just trying to clarify the issue title. Changes in the system lists are actually imported correctly already (just like any other config). The actual problem is that newly enabled extensions do not participate in the config staging/import process; i.e., their config import hooks are not invoked.

beejeebus’s picture

ack to #12. those tests pass locally for me :-(

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new8.32 KB
PASSED: [[SimpleTest]]: [MySQL] 42,815 pass(es).
[ View ]

aaaaaand they test locally because i forgot to git add, git commit the test module and yml file to my local branch.

new patch with those files.

beejeebus’s picture

Assigned:beejeebus» Unassigned
beejeebus’s picture

Priority:Major» Normal

downgrading so it doesn't block other features - we're right on 100 majors right now.

alexpott’s picture

StatusFileSize
new3.62 KB
FAILED: [[SimpleTest]]: [MySQL] 49,270 pass(es), 5 fail(s), and 3 exception(s).
[ View ]

This is an important issue and one of the reasons why I started on #1890784: Refactor configuration import and sync functions

Another thing we need to consider apart from just the config_import hooks are any install / disable / schema hooks. At the moment these are not fired at all.

The attached patch (which will fail) shows that after a config_import that updates system.modules - the module does not exist on the service, the schema is not installed and default config is not installed.

sun’s picture

Priority:Normal» Critical

I'm pretty sure we cannot release without this, so bumping to critical.

The latest patch in #15 looks a bit messy ;) but I can perfectly see that the required logic will be relatively ugly either way.

Given the discussion results of #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed — I'd suggest to simply and strictly deny importing any config in case any module is disabled. Removing that exceptionally horrible edge-case from the equation will likely make the logic a lot easier.

Furthermore, do we want to bake this into the existing syncing functions, or do we perhaps want to split the functionality into completely separate functions? (and change the existing to ignore those special config object names) I'd assume that splitting it out would help us to make the code a little bit cleaner?

We also need to update this for the new module_handler service.

tim.plunkett’s picture

Status:Needs work» Postponed
Issue tags:-Needs tests

Looking at the code in #15, and realizing we don't have a current solution for this yet, I think it would make sense to postpone this on #1890784: Refactor configuration import and sync functions. It will be easier to tackle once that is done.

I do agree that this is a critical that needs to be fixed.

Also, if #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed is decided, it will make this more simple.

beejeebus’s picture

Status:Postponed» Active

aaaaand that landed a while back, so this is open again.

alexpott has some code locally for this.

beejeebus’s picture

Title:Config import hooks of newly enabled extensions are not invoked in the config staging/import process» Add a separate module install/uninstall step to the config import process
Assigned:Unassigned» beejeebus

retitling. i'll try to get a patch up for this today.

alexpott’s picture

StatusFileSize
new17.08 KB
FAILED: [[SimpleTest]]: [MySQL] 55,718 pass(es), 0 fail(s), and 6,228 exception(s).
[ View ]

Patch attached adds

  • adds a step to handle any change to the system module files
  • adds a step to handle any change to the system theme files
  • a prioritisation event to config import so modules can reorder to the list config to import
alexpott’s picture

Status:Active» Needs review
alexpott’s picture

StatusFileSize
new16.71 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new4.93 KB

Removed prioritisation step as it is out-of-scope and untested...
Recalculating differences after enable / disable / uninstall...
Handling errors a bit better...

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new16.97 KB
FAILED: [[SimpleTest]]: [MySQL] 53,042 pass(es), 116 fail(s), and 70 exception(s).
[ View ]
new2.42 KB

Fixes installer and ensure that during a module or theme enable we don't end up in some mad recursion...

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new17.64 KB
PASSED: [[SimpleTest]]: [MySQL] 57,153 pass(es).
[ View ]
new523 bytes

And to fix the view installer...

alexpott’s picture

So at the moment this recalculates the changelist after installing modules and themes as these actions create config in the active store and therefore stuff is going to have changed - @beejeebus thinks this is the way to go. Last night I said I wasn't sure and now thinking more about it I'm convinced and here is why...

Scenario 1 - with code in #30

Starting condition DEV and LIVE matching environments from configuration perspective...

On DEV

  1. Enable taxonomy module which create views.view.taxonomy_term
  2. Rename views.view.taxonomy_term views.view.my_special_flower
  3. Export config

Then on LIVE import config

  1. Enables taxonomy module
  2. Which creates views.view.taxonomy_term
  3. Recalculates changelist
  4. Rename views.view.taxonomy_term views.view.my_special_flower

Scenario 2 - with code #30

Starting condition DEV and LIVE matching environments from configuration perspective...

On DEV

  1. Enable taxonomy module which create views.view.taxonomy_term
  2. Rename views.view.taxonomy_term views.view.myspcial_flower
  3. Rename views.view.myspcial_flower views.view.my_special_flwer
  4. Rename views.view.my_special_flwer views.view.my_special_flower
  5. Export config

Then on LIVE import config

  1. Enables taxonomy module
  2. Which creates views.view.taxonomy_term
  3. Recalculates changelist
  4. Rename views.view.taxonomy_term views.view.my_special_flower

The point being we now have not followed exactly the same set of renames on LIVE as on DEV.

Scenario 2 - how I think it should work

Starting condition DEV and LIVE matching environments from configuration perspective...

On DEV

  1. Enable taxonomy module which create views.view.taxonomy_term
  2. Rename views.view.taxonomy_term views.view.myspcial_flower
  3. Rename views.view.myspcial_flower views.view.my_special_flwer
  4. Rename views.view.my_special_flwer views.view.my_special_flower
  5. Export config

Then on LIVE import config

  1. Enables taxonomy module
  2. As this is an import module enable we ignore default config because we have a full config tree
  3. Create views.view.my_special_flower

I think the given that two systems A and B start in exactly the same state. System A can take what ever path it likes to get to a point where it exports. On importing system B should take the shortest possible path to get to the expected config. As there is absolutely no way we can know the exact path the system A took to get to the point where it was when it exports its config.

... however this still does not get away from the fact we still have to implement renames. Because of this situation... it is just a different problem and out-of-scope for dealing with module and theme enables...

Scenario 3 - why renames are vital

Starting condition DEV and LIVE matching environments from configuration perspective...

On DEV

  1. Enable taxonomy module which create views.view.taxonomy_term
  2. Export config

Then on LIVE import config

  1. Enables taxonomy module
  2. Creates views.view.taxonomy_term

Then on DEV

  1. Rename views.view.taxonomy_term views.view.my_special_flower
  2. Export config

Then on LIVE import config

  1. Enables taxonomy module
  2. Renames views.view.taxonomy_term views.view.my_special_flower
tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system, +Configurables
beejeebus’s picture

Assigned:beejeebus» Unassigned
beejeebus’s picture

i'm meh about #30 and #31, but it's green, so lets go with it.

tayzlor’s picture

StatusFileSize
new222.08 KB
new230.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,171 pass(es), 3 fail(s), and 624 exception(s).
[ View ]

Here's an updated patch. It follows more closely with "Scenario 2 - how I think it should work" in #31.
I've overridden the module handler to provide a config one, which is available during a config import. This allows us to bypass the config_install_default_config() call during a module enable, instead importing from the full config tree for the module that is being enabled.

To test this patch, what I was doing was getting a config export from a standard profile install, then doing a minimal installation, and trying to do a config import to take us from minimal > standard.

I've included an additional test that does just this, which should fail. This patch does the config import and manages to take us nearly all the way from minimal > standard, but there are some outstanding issues.

tayzlor’s picture

Status:Needs work» Needs review

Post save hooks are a bit of an issue. Couple of examples -

Custom_block module. It creates a field by doing custom_block_add_body_field(). if this field is coming in later on in the config import (from another config file) it will break.
Contact module - references $this->original->id() (where original may not actually exist if we are doing a config import)

Also because we are calling theme_enable() this installs the default config (as config_install_default_config() is embedded in that function). What we would possibly need here is a themeHandler class (similar to the moduleHandler), where we could do a similar override (to the one i've implemented in the ConfigModuleHandler) when we are enabling a theme via a config import.

tayzlor’s picture

Status:Needs review» Needs work
StatusFileSize
new236.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,135 pass(es), 3 fail(s), and 625 exception(s).
[ View ]

Since the node types conversion to CMI landed, here's an updated patch that includes those YAML files in the staging directory for the test minimal to standard config import.

andyceo’s picture

Status:Needs work» Needs review
tayzlor’s picture

This issue depends on #1969800: Add UUIDs to default configuration being committed.

beejeebus’s picture

EDIT - remove unhelpful comment.

beejeebus’s picture

David_Rothstein’s picture

Is there a reason this is postponed? It looks to me that disabled modules only come into play here via a few small lines of code in the handleModules() method.

beejeebus’s picture

Status:Postponed» Active

sigh.

beejeebus’s picture

Status:Active» Needs review
StatusFileSize
new9.07 KB
FAILED: [[SimpleTest]]: [MySQL] 58,630 pass(es), 0 fail(s), and 692 exception(s).
[ View ]

so here's a first cut that is almost certainly broken in important ways.

beejeebus’s picture

ViewTestConfigInstaller extends ConfigImporter. no, no, no, this is getting out of hand.

beejeebus’s picture

beejeebus’s picture

beejeebus’s picture

Status:Postponed» Needs review
StatusFileSize
new15.29 KB
FAILED: [[SimpleTest]]: [MySQL] 59,175 pass(es), 0 fail(s), and 30 exception(s).
[ View ]

i've updated the patch to include #2095489: Separate out module install config code from import code - i'd like to see how this runs now.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new52.48 KB
FAILED: [[SimpleTest]]: [MySQL] 50,706 pass(es), 936 fail(s), and 2,092 exception(s).
[ View ]

should be less fails with this one.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new16.78 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/config/lib/Drupal/config/Form/ConfigSync.php.
[ View ]

so. bad. at. this. game.

uploaded the wrong patch to #55. this is the one i meant.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new16.73 KB
FAILED: [[SimpleTest]]: [MySQL] 59,301 pass(es), 0 fail(s), and 16 exception(s).
[ View ]

less durp.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new16.99 KB
PASSED: [[SimpleTest]]: [MySQL] 58,842 pass(es).
[ View ]

even less durp.

beejeebus’s picture

bump.

beejeebus’s picture

StatusFileSize
new16.99 KB
PASSED: [[SimpleTest]]: [MySQL] 58,846 pass(es).
[ View ]

reroll.

beejeebus’s picture

i'll try to write a test for install/uninstall modules now that #2106171: Write tests for simple configuration deployment scenario has landed.

related - #2108813: Add fancier config import/export test scenario.

beejeebus’s picture

StatusFileSize
new19.25 KB
FAILED: [[SimpleTest]]: [MySQL] 59,054 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

this adds a test that just installs config_test.

fails with:

<?php
The config_test entity type does not exist
.    InvalidArgumentException    EntityManager.php    215    Drupal\Core\Entity\EntityManager->getControllerClass()
?>

this is why we can't have nice things.

Anonymous’s picture

Issue summary:View changes

mtift updated issue summary

alexpott’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

alexpott’s picture

chx’s picture

Allow me to express my doubts over the direct approach here. Shouldn't this work via some event? Of course it would require that modules CMI object get processed first -- as I expressed above, the CMI dependency is a problem.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new50.59 KB
FAILED: [[SimpleTest]]: [MySQL] 60,540 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
Gábor Hojtsy’s picture

@alexpott: are you working on this? Any feedback needed? :)

beejeebus’s picture

Assigned:Unassigned» beejeebus
Nitesh Sethia’s picture

Nitesh Sethia’s picture

Assigned:Nitesh Sethia» Unassigned
StatusFileSize
new2.63 MB
FAILED: [[SimpleTest]]: [MySQL] 60,628 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Patch has been rerolled.

Nitesh Sethia’s picture

Status:Needs work» Needs review
beejeebus’s picture

Assigned:Unassigned» beejeebus
beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new45.72 KB
FAILED: [[SimpleTest]]: [MySQL] 63,672 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

reroll based on #70. lets see where we're at.

yched’s picture

Hate to be dropping this so late :-/, but shouldn't we defer the uninstalls to the end of the sync process ?
- install modules that need to be installed according to system.module.yml
- sync the config tree other than system.module.yml
- only then, uninstall modules that need to be uninstalled according to system.module.yml
?

For the same reason "enable first" is needed so that we can handle the creation of the config entities owned by the "new" modules, I'd think we want to "disable last" so as to keep "old" modules active until we're done dealing with their config entities ?

alexpott’s picture

@yched good point! You're not late we're only just getting started with this issue :)

beejeebus’s picture

re. #81 - huh, took me a while to figure out what that meant, but yeah, yched++

i'll add that in to the next patch.

xjm’s picture

beejeebus’s picture

Assigned:beejeebus» Unassigned
jessebeach’s picture

Title:Add a separate module install/uninstall step to the config import process» [PP-1] Add a separate module install/uninstall step to the config import process
Issue summary:View changes
jessebeach’s picture

Title:[PP-1] Add a separate module install/uninstall step to the config import process» Add a separate module install/uninstall step to the config import process
Status:Postponed» Active

Unpostponed!

alexpott’s picture

Status:Active» Needs review
StatusFileSize
new49.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/forum/forum.install.
[ View ]

Rerolled patch - we need to consider how to do this now that the Config Importer is batched. Patch attached just does all the module / theme installing and uninstalling in BatchConfigImporter::initialize() which is wrong.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new407 bytes
new49.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new1.46 KB
new50.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,161 pass(es), 31 fail(s), and 0 exception(s).
[ View ]

So we need to ensure Extension storage has an up to date list of enabled modules and themes during regular default configuration import.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new2.19 KB

Fixed up some of the test fails.

settings.translation_sync issue will be resolved in #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration

alexpott’s picture

StatusFileSize
new2.19 KB
new51.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,952 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new7.96 KB
new60.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,347 pass(es), 363 fail(s), and 17 exception(s).
[ View ]

The views were being ordered inconsistently during import because the default configuration had inconsistent position values.

Fingers crossed this is green :)

@todo: implement sensible batching of extension install and uninstall during config import.

However the ConfigImportAll test passes!!!! This test installs the standard profile. Then installs all available modules. Exports the configuration to staging. Deletes all fields and then uninstalls all possible modules and then syncs the site using the exported config to restore everything..

sun’s picture

This test installs the standard profile. Then installs all available modules. Exports the configuration to staging. Deletes all fields and then uninstalls all possible modules and then syncs the site using the exported config to restore everything..

Hm. No (new) test should rely on Standard profile.

cf. #1595028: [meta] Convert tests using Standard profile to use Testing profile instead

Any chance to convert that into DUTB + test the actual regression vectors instead?

I'm explicitly stating DUTB, because this test does not and should not care for anything else than installing, importing, and exporting configuration.

That said, it is not clear to me why such a test has to install all available configuration of all available modules in the first place... Tests should exclusively focus on possible test cases (possibly permutations) only. Installing a bunch of random modules that happen to have some random configuration does not cover any particular test case and only slows down the overall test suite performance. Those are the kind of tests that are guaranteed to get into your way, and everyone will avoid to run them at all costs ('cos a single test run takes ages to complete).

So, what are the actual test cases to cover?

alexpott’s picture

@sun this explicitly tests that all non-required modules can be enabled through the config importer and that the default configuration created by a standard install can also by created reliably through the importer. If you do not want to rely on standard profile - what we could do is enable all the modules and then import the configuration from the standard profile. But in my mind this is still relying on the standard profile. Further more I'd like to add the converse test that all non-required modules and their configuration and config entities can be removed by the config importer.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new61.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,157 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
new3.34 KB
new61.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,153 pass(es), 0 fail(s), and 8 exception(s).
[ View ]

CommentManager::addDefaultField needs to hide the comment field on any existing entity displays. Interdiff impossible so a real diff of the two patches attached.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new677 bytes
new62.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,208 pass(es).
[ View ]

Fixing the new test

YesCT’s picture

Assigned:Unassigned» YesCT
Status:Needs review» Needs work
beejeebus’s picture

Assigned:YesCT» beejeebus

working on this.

beejeebus’s picture

Assigned:beejeebus» Unassigned
Status:Needs work» Needs review
StatusFileSize
new74.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,038 pass(es), 14 fail(s), and 5 exception(s).
[ View ]

current patch. import UI tests are failing, and i don't know how to fix it.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new21.23 KB
new76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,452 pass(es).
[ View ]

This patch adds:

  • sorting the modules in the correct order for install and uninstall
  • changing the admin theme as well as the default - for the same reasons this is important - what happens if we're processing a batch and the admin theme is disabled?
  • it makes the BatchConfigImporter and ConfigImporter use more of the same code for processing extensions
  • it adds back the recalculation of the changelist after the extensions have been processed since processing extensions uses configuration from staging and not an extension's default config

Mind you this patch is very very ugly at this point :(

alexpott’s picture

StatusFileSize
new6.16 KB
new75.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,425 pass(es).
[ View ]

Patch attached implements a better way of dealing with default and admin theme changes after theme enables and (more importantly) before theme disables.

alexpott’s picture

StatusFileSize
new18.89 KB
new76.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,489 pass(es).
[ View ]
Berdir’s picture

Wow, this is pretty crazy...

Looked through it, found a bunch of nitpicks and some minor issues, but I don't have any glorious ideas to make it all much easier and nicer...

Nice test coverage!

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -131,13 +182,28 @@ public function getStorageComparer() {

    +  protected function getEmptyExtensionsProcessedList() {
    +    return array(

    insertMissingDocumentationNitpickHere()

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -187,8 +253,129 @@ protected function setProcessed($op, $name) {
    +   * @return void

    I don't think we do this.

  3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -239,7 +429,8 @@ public function validate() {
           }
    -      $this->eventDispatcher->dispatch(ConfigEvents::IMPORT_VALIDATE, new ConfigImporterEvent($this));
    +      $importer_event = new ConfigImporterEvent($this);
    +      $this->eventDispatcher->dispatch(ConfigEvents::IMPORT_VALIDATE, $importer_event);
           $this->validated = TRUE;
         }

    Seems unrelated?

  4. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -253,13 +444,58 @@ public function validate() {
    +      // Installing a module can cause a kernel boot therefore reinject all the
    +      // services.
    +      $this->reInjectMe();

    Aw.

  5. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -341,4 +577,67 @@ public function alreadyImporting() {
    +   * @param $config_name
    +   *   The configuration object name.

    Missing type.

  6. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -341,4 +577,67 @@ public function alreadyImporting() {
    +      // drupal_flush_all_caches();

    Left-over?

  7. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -341,4 +577,67 @@ public function alreadyImporting() {
    +  }
    +
    +  protected function reInjectMe() {

    docs...

  8. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -130,6 +144,18 @@ public function installDefaultConfig($type, $name) {
    +          // happening) will be unstable after the module has been enabled and
    +          // before the config entity has been imported.
    +          if ($this->isSyncing) {
    +            continue;
    +          }

    @@ -138,6 +164,9 @@ public function installDefaultConfig($type, $name) {
                 $id = $entity_storage->getIDFromConfigName($name, $entity_storage->getEntityType()->getConfigPrefix());
                 $entity = $entity_storage->load($id);
    +            if ($this->isSyncing) {
    +              $entity->setSyncing(TRUE);
    +            }

    Those two checks seem the conflict, the second one will never run then?

  9. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -159,4 +188,48 @@ public function installDefaultConfig($type, $name) {
    +      // If using the the extension install storage class can not

    Cut off comment.

  10. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportAllTest.php
    @@ -0,0 +1,109 @@
    +    // Delete every field on the site so all modules can be disabled. For

    uninstalled ;)

    Also, I *thought* we fixed this so that fields are uninstalled correctly, but it's possibly that there are some left-overs...

  11. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportAllTest.php
    @@ -0,0 +1,109 @@
    +    \Drupal::moduleHandler()->uninstall(array_keys($all_modules));

    All modules aren't actually all modules anymore at this point, maybe use a different variable?

  12. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -65,16 +69,60 @@ function testImport() {
    +    // during the install. Options and Text are there in ensure modules are
    +    // installed with the correct dependencies. Options depends on Text so Text
    +    // should be installed first.

    first sentence seems to be missing some words or so ;) Also, those two are now already enabled for the test? and are not in the list?

  13. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -65,16 +69,60 @@ function testImport() {
    +    // Disable the Options and Text modules to ensure that dependencies are handled
    +    // correctly.
    +    \Drupal::moduleHandler()->uninstall(array('text', 'options'));

    Ah, here we go ;) Still, the comment above seems outdated.

  14. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -88,6 +136,85 @@ function testImport() {

         // Verify the cache got cleared.
         $this->assertTrue(isset($GLOBALS['hook_cache_flush']));
    +

    Not related, but pretty sure that this assertion is completely bogus. this happens inside the page request, it doesn't affect our global? It works because we do a dfac() during test setup I guess, should also be green when checking before you import ;)

  15. +++ b/core/modules/config/tests/config_import_test/lib/Drupal/config_import_test/EventSubscriber.php
    @@ -0,0 +1,101 @@
    +use Drupal\Core\KeyValueStore\StateInterface;
    +use Symfony\Component\EventDispatcher\EventSubscriberInterface;
    +
    +
    +/**

    two spaces :p

  16. +++ b/core/modules/config/tests/config_import_test/lib/Drupal/config_import_test/EventSubscriber.php
    @@ -0,0 +1,101 @@
    +  public function onConfigSave(ConfigCrudEvent $event) {

    needs some inheritdocs on this and other methods.

  17. +++ b/core/modules/config/tests/config_import_test/lib/Drupal/config_import_test/EventSubscriber.php
    @@ -0,0 +1,101 @@
    +    return $events;
    +  }
    +
    +}
    \ No newline at end of file

    .

  18. +++ b/core/modules/content_translation/content_translation.install
    @@ -87,6 +87,19 @@ function content_translation_install() {
    +  $config_names = \Drupal::configFactory()->listAll('field.field.');
    +  foreach ($config_names as $name) {
    +    \Drupal::config($name)
    +      ->set('settings.translation_sync', FALSE)
    +      ->save();
    +  }
    +  $config_names = \Drupal::configFactory()->listAll('field.instance.');
    +  foreach ($config_names as $name) {
    +    \Drupal::config($name)
    +      ->set('settings.translation_sync', FALSE)
    +      ->save();

    I guess this is a workaround until that is in it's own files?

  19. +++ b/core/modules/field/lib/Drupal/field/FieldInstanceConfigInterface.php
    index af5ebf2..1c7d8a4 100644
    --- a/core/modules/forum/config/field.field.forum.forum_container.yml

    --- a/core/modules/forum/config/field.field.forum.forum_container.yml
    +++ b/core/modules/forum/config/field.field.forum.forum_container.yml

    +++ b/core/modules/forum/config/field.field.forum.forum_container.yml
    +++ b/core/modules/forum/config/field.field.forum.forum_container.yml
    @@ -1,4 +1,4 @@

    @@ -1,4 +1,4 @@
    -id: taxonomy_term.forum_container
    +id: forum.forum_container

    The fix should be the other way round? entity_type.fieldname, and taxonomy_term is the entity type, not forum..

  20. +++ b/core/modules/forum/forum.install
    @@ -16,72 +16,75 @@ function forum_install() {
    +  if (!\Drupal::service('config.installer')->isSyncing()) {
    +    // Create the 'taxonomy_forums' field if it doesn't already exist. If forum
    +    // is being enabled at the same time as taxonomy after both modules have been
    +    // enabled, the field might exist but still be marked inactive.

    All those checks and edge cases are becoming more and more complicated :(

  21. +++ b/core/modules/node/node.install
    @@ -459,7 +459,9 @@ function node_uninstall() {
       $types = \Drupal::configFactory()->listAll('node.type.');
       foreach ($types as $config_name) {
         $type = \Drupal::config($config_name)->get('type');
    -    \Drupal::config('language.settings')->clear('node. ' . $type . '.language.default_configuration')->save();
    +    if (\Drupal::moduleHandler()->moduleExists('language')) {
    +      \Drupal::config('language.settings')->clear('node. ' . $type . '.language.default_configuration')->save();
    +    }
       }

    Unrelated but this could use hook_entity_bundle_delete() in language.module now, which is triggered by entity_module_preuninstall().

    There's also talk of moving those settings into the language field settings.

alexpott’s picture

StatusFileSize
new11.54 KB
new77.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,447 pass(es).
[ View ]

Thanks @Berdir.

1 - 17. All fixed
18. Yep :(
19. Yes... nice catch
20. Indeed
21. Agreed but out of scope :)

beejeebus’s picture

alexpott++
berdir++

<?php
+      if (!empty($operation['type'])) {
+       
$this->processExtension($operation['type'], $operation['op'], $operation['name']);
+       
$this->recalculateChangelist = TRUE;
+      }
+      else {
+        if (
$this->recalculateChangelist) {
+         
$current_total = 0;
+          foreach (array(
'create', 'delete', 'update') as $op) {
+           
$current_total += count($this->getUnprocessedConfiguration($op));
+          }
+         
$this->storageComparer->reset();
+         
// This will cause the changelist to be calculated.
+          $new_total = 0;
+          foreach (array(
'create', 'delete', 'update') as $op) {
+           
$new_total += count($this->getUnprocessedConfiguration($op));
+          }
+         
$this->totalToProcess = $this->totalToProcess = $current_total + $new_total;
+         
$operation = $this->getNextOperation();
+         
$this->recalculateChangelist = FALSE;
+        }
+       
// Rebuilding the changelist change remove all changes.
+        if ($operation !== FALSE) {
+         
$this->processConfiguration($operation['op'], $operation['name']);
+        }
+      }
?>

i found this really hard to follow. can we break it up in to some methods with names? or add a big docblock or something? i don't even know what to suggest, because i just don't follow what it's doing.

alexpott’s picture

StatusFileSize
new12.77 KB
new81.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,419 pass(es).
[ View ]

How about breaking the batch up into truly separate steps for extensions and configuration - the attached patch does this and I think it is easier to understand.

beejeebus’s picture

nice! #116 is much clearer. i think this is getting close, but i don't think i should RTBC it.

alexpott++

alexpott’s picture

StatusFileSize
new1.41 KB
new83.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,508 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Been testing this patch by doing the following:

  1. Apply the latest patch from #2232275: System menu blocks need to be able to depend on their menu entities
  2. Install the standard profile
  3. Copy the active config directory somewhere safe
  4. Edit the copied version of core.extension.yml to use the minimal profile instead of standard
  5. Destroy the site
  6. Install the minimal profile
  7. Copy the saved standard config to staging
  8. Copy system.site.yml from active to staging
  9. Run the import http://www.screencast.com/t/NXI1Xu5VQ3.

Fixed an interesting issue with recreates going in in the wrong order due to being added after the creates and deletes.

Status:Needs review» Needs work

The last submitted patch, 118: 1808248.118.patch, failed testing.

sun’s picture

This looks great! Reviewed the patch/code (did not apply and/or test it, but #118 is more than sufficient proof for me :-))

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -90,6 +106,27 @@ class ConfigImporter extends DependencySerialization {
    +   * Flag set to import system.theme during processing theme enable and disables.

    s/system.theme/core.extension/ ?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -131,13 +175,34 @@ public function getStorageComparer() {
       public function reset() {

    Shouldn't this also reset $processedSystemTheme?

  3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -187,8 +252,139 @@ protected function setProcessed($op, $name) {
    +  public function getUnprocessedConfiguration($op) {

    In reviewing this patch, I found all the "Unprocessed" method + variable names very confusing — they sound as if they're about an unprocessed data/domain object.

    Would be great to rename all of them into "Pending" (or similar).

    Briefly discussed with @alexpott in IRC... let's do that in a follow-up issue.

  4. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -208,19 +406,20 @@ public function import() {
    +      // Where to put this?
    +      $this->handleExtensions();

    Is this @todo still an open question?

  5. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -253,13 +452,58 @@ public function validate() {
    +      // Installing a module can cause a kernel boot therefore reinject all the
    +      // services.
    +      $this->reInjectMe();
    +      // During the container is rebuilt and the module handler is called from
    +      // drupal_get_complete_schema(). This causes the container's instance of
    +      // the module handler not to have loaded.
    +      $this->moduleHandler->loadAll();

    Oh wow.

    Since the workarounds appear to work for now, let's create separate follow-up issues to investigate whether we can resolve this cleaner/differently?

  6. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -130,6 +144,18 @@ public function installDefaultConfig($type, $name) {
    +          // If we are syncing do not create configuration entities. Pluggable
    +          // configuration entities can have dependencies on modules that are
    +          // not yet enabled. In the absence of dependency management for config
    +          // entities this is a good as we can do. The problem with this
    +          // approach is that any code that expects default configuration
    +          // entities to exist (even if there is code the prevents this from
    +          // happening) will be unstable after the module has been enabled and
    +          // before the config entity has been imported.
    +          if ($this->isSyncing) {
    +            continue;
    +          }

    This comment (at least the parts about an absence of dependencies) sounds obsolete, no?

  7. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
    @@ -118,11 +118,16 @@ public function getChangelist($op = NULL) {
    +  protected function addChangeList($op, array $changes, array $sort_order = NULL) {
    ...
    +    if (!empty($sort_order)) {

    There's a small mismatch here:

    If it's legit to pass an empty array for $sort_order, then the condition should be isset().

    Otherwise, $sort_order should probably default to an empty array.

  8. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -582,6 +582,12 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +    /** @var \Drupal\Core\Config\ConfigInstaller $config_installer */
    +    $config_installer = \Drupal::service('config.installer');
    +    $sync_status = $config_installer->isSyncing();
    +    if ($sync_status) {
    +      $source_storage = $config_installer->getSourceStorage();
    +    }

    @@ -671,6 +677,18 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +        $config_installer = \Drupal::service('config.installer');
    +        if ($sync_status) {
    +          $config_installer
    +            ->setSyncing(TRUE)
    +            ->setSourceStorage($source_storage);
    +        }
    +        else {
    +          // If we're not in a config synchronisation reset the source storage
    +          // so that the extension install storage will pick up the new
    +          // configuration.
    +          $config_installer->resetSourceStorage();
    +        }

    Hm. Doesn't the ConfigInstaller / ConfigImporter handle this already?

    Also, wondering why we're not simply tagging config.installer with 'persist' instead?

    Wouldn't that resolve all of the manual futzing with re-injecting the previous state into the config installer/importer...?

    Wasn't this in-process rebuild from ModuleHandler the exact reason for why we introduced the 'persist' tag for services?

  9. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -146,6 +146,11 @@ public function enable(array $theme_list) {
    +      // If we're not in a config synchronisation reset the source storage so
    +      // that the extension install storage will pick up the new configuration.
    +      if (!$this->configInstaller->isSyncing()) {
    +        $this->configInstaller->resetSourceStorage();
    +      }

    Oh wow. I wonder why I didn't run into this problem in #1067408: Themes do not have an installation status ...?

    At least I assume that comment to actually try to say:

    "ExtensionInstallStorage only knows about the currently enabled list of themes, so it has to be reset in order to pick up the default config of the newly installed theme. However, do not reset the source storage when synchronizing configuration, since that would needlessly trigger a reload of the whole configuration to be imported."

    No? Wanna take over that as a replacement comment? :-)

  10. +++ b/core/modules/config/tests/config_import_test/config_import_test.services.yml
    @@ -0,0 +1,6 @@
    \ No newline at end of file

    minor

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new4.95 KB
new84.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,461 pass(es).
[ View ]

Thanks @sun

  1. Nope this is about change the default and admin themes at the right time - which, if we are disabling themes, is right before we do the disabling
  2. Nice catch
  3. Sure
  4. Nope
  5. Yeah this is difficult
  6. Improved the comment
  7. Fixed - improved doc and got a bit defensive as we do not want sorting to mangle the data
  8. Please no persist :) it is doing this because it has to ensure that a kernel reboot and container rebuild does not change this. Persisting the config installer would mean that all the services it depends have to be persisted to - this is why I think it is a terrible idea.
  9. Fixed
  10. Fixed

Fixed the test fails. These where due the sorting in StorageComparer::addChangeList() making the array keys not start from 0.

sun’s picture

OK... I don't really agree with the approach taken just to avoid 'persist', but I guess we should simply move forward with this.

+++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -119,14 +121,21 @@
+        throw \InvalidArgumentException(String::format('Sorting the @op changelist should not change its length.', array('@op' => $op)));

throw *new* ? :)

alexpott’s picture

StatusFileSize
new737 bytes
new84.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,457 pass(es).
[ View ]

Doh!

So the other reason for not using persistent services is that it means that a module can not ever swap out the service. We'll just copy the old one over.

sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks! — Yeah, OK, let's move forward here.

The topic of persist + kernel rebuilds is a super complex topic of its own and definitely shouldn't hold up this patch.

alexpott’s picture

StatusFileSize
new23.73 KB
new810 bytes
new84.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,553 pass(es).
[ View ]

Improved batch progress message.

See http://www.screencast.com/t/NXI1Xu5VQ3 for incorrect message.

Berdir’s picture

I didn't do an in depth review of the latest patches, but +1 from my side, can't spot anything that is wrong and if sun and beejebus are OK with it then I am too :)

catch’s picture

Discussed some of the issues like the order of install/uninstall with Alex last week and have no issues.

Overall this looks great. I found some very minor nitpicks that we could handle in a follow-up, posting here rather than ignoring though. The container syncing issue is horrible but not at all the fault of this patch, we've got other issues open trying to figure that out.

  1. +++ b/core/lib/Drupal/Core/Config/BatchConfigImporter.php
    @@ -37,49 +75,99 @@ public function initialize() {
    +  public function processConfigurationBatch(array &$context) {

    Missing docs.

  2. +++ b/core/lib/Drupal/Core/Config/BatchConfigImporter.php
    @@ -37,49 +75,99 @@ public function initialize() {
    +  public function finishBatch(array &$context) {

    Also here.

  3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -253,13 +453,58 @@ public function validate() {
    +      // During the container is rebuilt and the module handler is called from

    During a container rebuild the module handler is called? Couldn't parse this. This should be fixed by #2194785: [meta] Stop relying on database schema info at runtime so could add a @todo.

If I have time for another read through, I'll commit this a bit later today, shouldn't stop another maintainer committing it if they get back here first though :)

alexpott’s picture

StatusFileSize
new1.97 KB
new84.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,505 pass(es).
[ View ]

Fixed nits from #127

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

  • Commit ae702dc on 8.x by catch:
    Issue #1808248 by alexpott, beejeebus, tayzlor, Nitesh Sethia: Add a...
dawehner’s picture

This issue seems to be a potential source for "random" failures: https://qa.drupal.org/pifr/test/764353 is one example but I have seen that elsewhere.
I wonder whether we could file a followup which don't enables all the modules at once. On top of that it would be great to exclude contrib modules in sites/*/modules or modules/

YesCT’s picture

I'm going to open the follow-up for #98 "@todo: implement sensible batching of extension install and uninstall during config import." right now.
just putting the structure in place to track them in the issue summary.

--
Also, quickly noting re #131, #2233929: drupal_set_time_limit should only be able to increase the time limit can deal with it (but the #98 follow-up might actually deal with the cause).

YesCT’s picture

Issue summary:View changes

that was already a child. not sure if it needs to be related also. but these new fields are still new... not sure how best to use them.

updating the issue summary to note they are follow-ups though.

tim.plunkett’s picture

effulgentsia’s picture

shouldn't we defer the uninstalls to the end of the sync process?

Was this implemented? If not, why not? If so, how? Looking at the code, I only see:

if ($this->totalExtensionsToProcess > 0) {
  $sync_steps[] = 'processExtensions';
}
$sync_steps[] = 'processConfigurations';

which from what I can tell, does installs and uninstalls in the same place, and before configs.

I'm curious, because #2198429: Make deleted fields work with config synch just landed, which front loads the purging to the beginning of the process, rather than between processConfigurations() and (the nonexistent) processUninstalls().

alexpott’s picture

No config uninstalls are not deferred. This is because they could affect config creation - that's why uninstalls are done first before installs. The other thing you is the final environment the config export came from - it is important to get to the same code base as soon as possible. I'm going to open an issue to add documentation to the ConfigImporter class about priority and ordering of everything.

effulgentsia’s picture

it is important to get to the same code base as soon as possible

That makes sense. Thanks.

sun’s picture

Status:Fixed» Closed (fixed)

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