Fixes for migrating from legacy version #979
Conversation
Previously, any form submission would persist the database settings giving no chance to validate them or edit them. Toggling the MySQL enabled checkbox also triggered this as that reloaded the page.
On the initial load the form morphology hook is called prior to the morphology being populated with values from the model. Loading from the YAML won't always work if we're coming from < 0.7.0. Loading from the model is safe as: - The model is initially populated with the YAML - If legacy config is present then the model is updated with that - If this isn't the initial load we go down the submitted branch instead
| $oStandardConfig = new \Baikal\Model\Config\Standard(); | ||
| $oStandardConfig->set("configured_version", BAIKAL_CONFIGURED_VERSION); | ||
| $oStandardConfig->persist(); | ||
| } |
There was a problem hiding this comment.
Was my assumption right that this config init above leads to just parsed/migrated MySQL settings from config.system.php being overwritten by parsing the baikal.yaml which contains empty MySQL settings?
There was a problem hiding this comment.
baikal.yaml with empty Sqlite/MySQL settings is created during the form submission of the previous step at https://github.com/sabre-io/Baikal/blob/master/Core/Frameworks/BaikalAdmin/Controller/Install/Initialize.php#L76 but with a configured_version of 0.7.1 (the default inside the model).
At the next page load https://github.com/sabre-io/Baikal/blob/master/Core/Frameworks/BaikalAdmin/WWWRoot/install/index.php#L84 sees that the yaml is configured with the current Baikal version (0.7.1) but the INSTALL_DISABLE file isn't present yet, so we went up in this file.
The change you've highlighted here keeps configured_version at 0.7.1 in baikal.yaml until we've also written the Sqlite/MySQL settings to baikal.yaml (the additions in this commit are inside a $this->oForm->persisted() branch). configured_version in baikal.yaml is then rolled back to the value in config.system.php so that https://github.com/sabre-io/Baikal/blob/master/Core/Frameworks/BaikalAdmin/WWWRoot/install/index.php#L84 goes forward.
|
The encryption key is freshly created here, so it should be okay to not migrate it: Baikal/Core/Frameworks/BaikalAdmin/Controller/Install/Initialize.php Lines 75 to 77 in d87f028 |
I couldn't see where I have a local change to migrate |
That's an interesting find. Even when looking at the whole commit history ( @evert do you know why this config option was originally added? |
|
Sorry for the delay and thanks a lot for working on this. The PR looks good to me. Ready for merging, @benbz? |
|
Awesome, thanks! |
Tested with a MySQL DB dump & config of a 0.3.5 instance.
I'm able to:
Things still not migrated:
BAIKAL_ENCRYPTION_KEY- this doesn't appear to be read anywhere, so I didn't bother fixing the migration for that. If you desire I can update this PR to either migrate it (if there's some use I haven't spotted) or stop storing it altogether.BAIKAL_CARD_BASEURI,BAIKAL_CAL_BASEURI&BAIKAL_DAV_BASEURI- the use of these from card.php, cal.php & dav.php respectively appears to have been removed. In my legacy instance all 3 of these were set toPROJECT_BASEURI. This allowed my nginx config to rewrite any request that wasn't to/adminor/resto/dav.php, so thatdav.phpdidn't show up in the URL. I haven't figured out how to replicate this in 0.7.1 but I feel that is a separate problem to what I'm solve with this PR.Fixes #942 #946 I think