The provision_drupal_sync_site function seems to do too much.

Contrary to it's name it's also used to sync a platform.
This does need to be tested for side-effects!

This bugged my while working on #1083366: Make the spokes authoritative for files/ and private/ directories

CommentFileSizeAuthor
#4 Refactor_provision_drupal_sync_site-1803074-4.patch1.69 KBhelmo
provision-refactor-provision_drupal_sync_site.patch1.8 KBhelmo

Comments

anarcat’s picture

anarcat commented
Status: Needs review » Needs work

Good catch. However, why keep on calling _platform() from _site() if you do the rename? Call both upwards in the stack and you're done! If you make sure you cover all incantations in core, then we make an API change update for 2.x and we're done.

Thanks for the patch.

helmo’s picture

helmo commented

Well that's why I added "TODO: is this really neccecary?" in the patch. While working on this I wasn't entirely sure what to do.

anarcat’s picture

anarcat commented

Instead of this:

+++ b/provision.incundefined
@@ -183,7 +183,7 @@ function provision_save_platform_data() {
-    provision_drupal_sync_site();
+    provision_drupal_sync_platform();

... just do this:

provision_drupal_sync_site();
provision_drupal_sync_platform();

and don't call sync_platform() from sync_site(). No?

helmo’s picture

helmo commented
Status: Needs work » Needs review
StatusFileSize
new Refactor_provision_drupal_sync_site-1803074-4.patch1.69 KB

Then this should do it.

I did another grep though my sources and couldn't find any other places where this is used. So we should be OK.

anarcat’s picture

anarcat commented
Status: Needs review » Needs work 
+++ b/platform/provision_drupal.drush.incundefined
@@ -92,19 +92,24 @@ function drush_provision_drupal_provision_install_backend() {
-  if (d()->type === 'site') {
-    // Sync all filesystem changes to the remote server.

this is gone? seems to me the new logic is to sync the site even for platforms now... have you tested this?

helmo’s picture

helmo commented

I did some testing in a mixed 1.x/2.x setup.... I really should make some time to get a full aegir 2.x test server vm up...

+++ b/provision.inc
@@ -183,6 +183,7 @@ function provision_save_platform_data() {
     $config->write();
+    provision_drupal_sync_platform();
     provision_drupal_sync_site();

Instead of just adding the _platform call here my Initial patch had replaced _site with _platform. Looks likt #3 inspired me, but it seems odd reading it now.

helmo’s picture

helmo commented

I've been working with http://drupal.org/project/vagrant_scripts_aegir to get my own test bot...

Good news is, they pass with the patch from #4

But it's still odd.

anarcat’s picture

anarcat commented

My point is that there is a conditionnal around the site sync that is gone, now we sync files on sites even on platforms. This may not fail in the test suite because we don't test remote servers, but I think the conditionnal should be added back in.

helmo’s picture

helmo commented
Status: Needs work » Closed (won't fix)

Well never mind, this is probably not worth the effort.

Feel free to re-open ...

helmo’s picture

helmo commented
Issue summary: View changes

typo