ImportFolderDeletion failing on bizarre comparison result ?

[liverpoolfcfan]

I have implemented the code for DeleteFolder in the zimbra backend which in turn uses DiffBackend for processing changes. In testing I have come across a bizarre occurrence - at least it is bizarre as far as my PHP knowledge goes.

Here is the standard diffbackend.php code for ImportFolderDeletion

Code: Select all

    function ImportFolderDeletion($id, $parent) {
        //do nothing if it is a dummy folder
        if ($parent == SYNC_FOLDER_TYPE_DUMMY)
            return false;

        $change = array();
        $change["id"] = $id;

        $this->updateState("delete", $change);

        $this->_backend->DeleteFolder($parent, $id);

        return true;
    }


which is called from request.php by function HandleFolderCreate

Code: Select all

    if (!$delete) {
            // Send change
            $serverid = $importer->ImportFolderChange($serverid, $parentid, $displayname, $type);
    }
    else {
        // delete folder
        $deletedstat = $importer->ImportFolderDeletion($serverid, 0);
    }



So - it is getting called with a folderID in $serverid - which passes to $id in DiffBackend, and a ZERO in the other parameter which passes to $parent in DiffBackend

The function ImportFolderDeletion is returning immediately - without doing anything.

To debug, I added debug some code.

Code: Select all

    function ImportFolderDeletion($id, $parent) {
      debugLog( 'First test if  parent == DUMMY? ' );
      if ($parent == SYNC_FOLDER_TYPE_DUMMY) {
         debugLog( 'ImportFolderDeletion!' );
         debugLog( 'parent == DUMMY !' );
         debugLog( 'Parent ['.$parent.']' );
         debugLog( 'DUMMY ['.SYNC_FOLDER_TYPE_DUMMY.']' );
      } else {
         debugLog( 'parent != DUMMY !' );
      }

      debugLog( 'Now test if  DUMMY == parent? (in case there is strange type casting happening)' );
      if (SYNC_FOLDER_TYPE_DUMMY == $parent) {
         debugLog( 'DUMMY == parent !' );
         debugLog( 'Parent ['.$parent.']' );
         debugLog( 'DUMMY ['.SYNC_FOLDER_TYPE_DUMMY.']' );
      } else {
         debugLog( 'DUMMY != parent !' );
      }
        //do nothing if it is a dummy folder
        if ($parent == SYNC_FOLDER_TYPE_DUMMY)
            return false;
....



And here is what I found.

For some bizarre reason the code is matching a $parentid of 0(ZERO) with SYNC_FOLDER_TYPE_DUMMY which is "__dummy.Folder.Id__" no matter which way round the comparison is done.

As a result of matching these - the function returns false without actually doing anything.

... [myUser] First test if parent == DUMMY?
... [myUser] ImportFolderDeletion!
... [myUser] parent == DUMMY !
... [myUser] Parent [0]
... [myUser] DUMMY [__dummy.Folder.Id__]
... [myUser] Now test if DUMMY == parent?
... [myUser] DUMMY == parent !
... [myUser] Parent [0]
... [myUser] DUMMY [__dummy.Folder.Id__]

Does anyone have a good explanation for why this is happening ?


Additionally, I think there is a further problem in the ImportFolderDeletion function - in that it should be operating on the basis of the return from the backend - not independently.

Rather than

Code: Select all

        $change = array();
        $change["id"] = $id;

        $this->updateState("delete", $change);

        $this->_backend->DeleteFolder($parent, $id);

        return true;


which tells the phone to do the deletion (updateState "delete") regardless of what the backend thinks - I believe it should be

Code: Select all

        $change = array();
        $change["id"] = $id;

        $deleted = $this->_backend->DeleteFolder($parent, $id);

        if ($deleted) {
            $this->updateState("delete", $change);
            return true;
        } else {
            return false;
        }


which looks to the backend to make the determination as to whether the phone is allowed to delete the folder or not.

[mku]

Hi liverpoolfcfan,

For some bizarre reason the code is matching a $parentid of 0(ZERO) with SYNC_FOLDER_TYPE_DUMMY which is "__dummy.Folder.Id__" no matter which way round the comparison is done.

[...]

Does anyone have a good explanation for why this is happening ?

Yes, that's how PHP handles types. Please take a look here: http://de2.php.net/manual/en/language.types.string.php#language.types.string.conversion and here http://de2.php.net/manual/en/language.types.type-juggling.php and here http://de2.php.net/manual/en/types.comparisons.php. The strict comparison would return the expected result.

Additionally, I think there is a further problem in the ImportFolderDeletion function - in that it should be operating on the basis of the return from the backend - not independently.

Rather than

Code: Select all

        $change = array();
        $change["id"] = $id;

        $this->updateState("delete", $change);

        $this->_backend->DeleteFolder($parent, $id);

        return true;


which tells the phone to do the deletion (updateState "delete") regardless of what the backend thinks - I believe it should be

[code]        $change = array();
        $change["id"] = $id;

        $deleted = $this->_backend->DeleteFolder($parent, $id);

        if ($deleted) {
            $this->updateState("delete", $change);
            return true;
        } else {
            return false;
        }


which looks to the backend to make the determination as to whether the phone is allowed to delete the folder or not.

That's Z-Push 1.5, right? Because Z-Push 2 handles it differently and takes the backend's return value of DeleteFolder into account.

Greets, Manfred

[liverpoolfcfan]

Yes it is 1.5

Given that this is the way PHP string comparison is supposed to work - clearly the delete folder code using diffbackend.php called from request.php can never work. Right ?

I see in zpushdefs.php from z-push-2 that the definition of SYNC_FOLDER_TYPE_DUMMY has been changed.
From

define("SYNC_FOLDER_TYPE_DUMMY", "__dummy.Folder.Id__");

to

define("SYNC_FOLDER_TYPE_DUMMY", 999999);

That presumably gets over the problem of the loose type comparisons not working - Right ?

Is that a valid fix for z-push 1.5 in this instance ?

Can you please provide a fix for diffbackend.php also ?

[mku]

Hi liverpoolfan,

Diffbackend and classes extending it do not have DeleteFolder. As 1.X is a dead end anyway and will be replaced by Z-Push 2 it doesn't make any sense to add new functionality to it.

Greets, Manfred

[liverpoolfcfan]

Hi liverpoolfan,

Diffbackend and classes extending it do not have DeleteFolder. As 1.X is a dead end anyway and will be replaced by Z-Push 2 it doesn't make any sense to add new functionality to it.

Greets, Manfred

It does ... here is the Class in diffbackend.php that calls it.

It is in the function ImportFolderDeletion($id, $parent) below that the problems lie.

Code: Select all

class ImportHierarchyChangesDiff extends DiffState {
    var $_user;

    function ImportHierarchyChangesDiff($backend) {
        $this->_backend = $backend;
    }

    function Config($state) {
        $this->_syncstate = unserialize($state);
    }

    function ImportFolderChange($id, $parent, $displayname, $type) {
        //do nothing if it is a dummy folder
        if ($parent == SYNC_FOLDER_TYPE_DUMMY)
            return false;

        if($id) {
            $change = array();
            $change["id"] = $id;
            $change["mod"] = $displayname;
            $change["parent"] = $parent;
            $change["flags"] = 0;
            $this->updateState("change", $change);
        }

        $stat = $this->_backend->ChangeFolder($parent, $id, $displayname, $type);

        if($stat)
            $this->updateState("change", $stat);

        return $stat["id"];
    }

    function ImportFolderDeletion($id, $parent) {
        //do nothing if it is a dummy folder
        if ($parent == SYNC_FOLDER_TYPE_DUMMY)
            return false;

        $change = array();
        $change["id"] = $id;

        $this->updateState("delete", $change);

        $this->_backend->DeleteFolder($parent, $id);

        return true;
    }
};


[mku]

Hi liverpoolfcfan,

yes, the Diffbackend calls it but it is not implemented in any class. Greping for DeleteFolder in the backend folder will only find it once - just that call.

Greets, Manfred

[liverpoolfcfan]

The point is that I, and I'm sure many other people have based their backends on diffbackend for managing the changes. And this function is broken in diffbackend

I have implemented the DeleteFolder function in my backend and it works as it should IF it gets called. But because of the problem in diffbackend.php

Code: Select all

        //do nothing if it is a dummy folder
        if ($parent == SYNC_FOLDER_TYPE_DUMMY)
            return false;


where 0 is being compared to __dummy.Folder.Id__ and matching - the backend function never gets called.

I am not asking you to implement a backend funcion - just to fix this ImportFolderDeletion function bug that prevents the backend function from being called.

I believe that changing the definition of SYNC_FOLDER_TYPE_DUMMY to 99999 as in z-push-2 would fix the initial problem. Would that break anything else ?


And changing the function ImportFolderDeletion so that it calls the backend first to determine if it should allow the deletion would solve the other part.

Code: Select all

function ImportFolderDeletion($id, $parent) {
        //do nothing if it is a dummy folder
        if ($parent == SYNC_FOLDER_TYPE_DUMMY)
            return false;

        $change = array();
        $change["id"] = $id;

        $deleted = $this->_backend->DeleteFolder($parent, $id);

        if ($deleted) {
            $this->updateState("delete", $change);
            return true;
        } else {
            return false;
        }
};


[mku]

Hi liverpoolfcfan,

it might break the old ActiveSync 1.0 devices. However probably there aren't a lot of them. But anyway Z-Push 1 will deprecate in the near future, so it doesn't make any sense to develop for it.

Greets, Manfred

[liverpoolfcfan]

I wasn't developing for 1.5 - I actually built the backend functions for 2 but then saw that the diffbackend for 1.5 had also got the ChangeFolder and DeleteFolder calls in it - so put the code into the 1.5 backend too.

That's when I ran into the bug.

It seems like it should not be too difficult to address. And with z-push-2 scheduled for 31-12-12 people will be using 1.5 for quite a while still - so probably worth fixing it.

[mku]

Hi liverpoolfcfan,

well, the system requires a release date, so 31-12-12 is kind of a place holder as the whole 2.X release is. The first stable version should be available way earlier.

Greets, Manfred