check for uuid before deleting otherwise if uuid does not exist the anonymous user is deleted along with associated content

I might make a small change to the patch, I just ran a test case it only needs to check for empty()
no need to test for ==false , as entity_get_id_by_uuid returns an empty array if the uuid is not found, false is never returned.

Here's the test case I wrote to verify this as it wasn't explicitly written on the api page:

$uuid=array('8e2a5916-c1b8-464a-831b-e5aa4ace99d4');
$test=entity_get_id_by_uuid($uuid);

print_r($test);
if (empty($test)) {
  echo "empty";
} elseif ($test==false) {
  echo "false";
} else {
  echo "uuid found";
}

Test results:
if the uuid isn't found the string "empty" is outputted

if the variable $test is not empty the string "uuid found" is outputted.

So, test for empty() , the test for ==false is not required

either way this patch works and it's critical if you want to protect your data from "oops"

Here is a simplified, cleaned up version of this patch for the latest dev build of uuid_services (sub module of uuid)

Hi Dave, thanks for the improved patch , explanation and commit. Great work, glad we could help out!

Hi Skwashd, after reflecting on the recent committed changes, I have some concerns having not yet tested it against our stack.

I'm not so concerned with your code as much as I am with the calling functions and callbacks. For this reason I am going to run tests against the new code. While I do understand your reasoning and it does seem logical, throwing the exception works, we did extensive testing with the patch from comment 5 (and we're using it in a production environment) and as I see it this is an alternate use case (for example we forgot to migrate the uuid when migrating users from our external "destination" site to our source site) that one would expect some sort of exception that in this case is gracefully handled by the rest of the system (at least it is with our stack). We didn't realize that we had improperly migrated our users until we encountered the problems prior to patching. We tested the patch from comment 5 extensively and it's behaving correctly. While the change from ! to @ in the message is an improvement returning true value is something we haven't tested and something that we will have to test.

Monday I'll take some time to spin up my test environment with the committed code to make sure that the rest of the system calls behave correctly without the exception throw.

So, I'll test the new code extensively on monday. It seems logical that it should also work however a real-world test and it's results will have the final say.

Thanks again,

Hi Dave, I ran some tests on the new code and the new code fails but I will have to re-test to tell you why and how. The patch from comment # 5 works but I tested the new code that was committed and it failed. I'll re-run a few tests tomorrow just to be sure.

This was a big issue for us so I'd like to be 100% sure that it's resolved. Please allow me some time to re-run some tests again tomorrow.

Thanks,

The exception needs to be thrown due to the callbacks and other parts of the system. I've tested and retested this. Patch 5 works, but commit 0b32fc7 does not avoid the problem that we have documented.

I've used some of Skwashd's improvements to create this new patch but the exception throw *MUST* remain.

Please commit the attached patch in place of 0b32fc7 on 7.x-1.x

patch 5 was rtbc+1 , patch 13 is to fix commit 0b32fc7

thanks,

Hi Dave, while the actual problem doesn't seem to be in uuid_services (and I haven't found the exact real SOURCE of the problem, but we DO know that throwing the exception when trying to delete a uuid that doesn't exist DOES AVOID THE PROBLEM COMPLETELY and works. However, I tested the new code from commit 0b32fc72 as you committed , it doesn't work properly with our stack and configuration given the fact that my test environment has users copied from the destination lacking uuid mapping (new uuid's, different on both sites for the same users but different uuid's) (using wetkit_deployment , wetkit_deployment_source and wetkit_deployment_destination). The callback doesn't catch an exception and abort it's behavior because no exception is thrown (you chose not to put a throw exception) when using the code from commit 0b32fc72 and continues destroying my test site as documented previously. The code from patch 13 works according to my tests.

We cannot accept code that given the test case and configuration results in massive amounts of entities deleted.

I will retest again using a full clone of the uuid module and see if the same test results in the destruction of site content. I'd like to find the true source of the problem but patch 13 and patch 5 are a good workaround that actually behave properly. We're using a version of patch 5 in production and it works. Now it seems like you're asking me to have to dig deeper. I'm not sure what your test environment looks like but perhaps you could run this test:

1) Set up a source and destination environment using deploy
2) you're using uuid for entities and users
3) on the "source" site db modify the uuid of your "test_user" directly in the users table , change it to a uuid that doesn't exist on your destination site
4) on the "source" site run user-cancel either from the GUI or from drush on the "test_user" user
5) after you run user-cancel, check your database monitor on the "destination" site (in phpmyadmin 4.x there's a mysql status section, look for database activity, wait about 5 minutes for this to stop)
6) still on the "destination" site; after 5 minutes, run "drush status" , see if you get a message about your anonymous user being deleted, check your users table, look for the anonymous user 0 , do you have a user with uid=0 ? If not, then any content that was attributed to the anonymous user will have been deleted. If you have users migrated from your destination site that lacked a uuid mapping during migration , you use the site for a few months, content created by a migrated user accumulates under the anonymous account , then once anonymous is deleted the content also is deleted.

patch 5 and 13 avoid this problem altogether

I will retest this type of environment with a few different configurations and see what happens.

Thanks

Ok, more test results, this time instead of applying the code changes just to the function, I replaced our uuid module with the current dev version.

the good news is, this issue is fixed when using the latest dev build of uuid (latest as of today oct 24, 2014)

Ironically an exception is correctly thrown in services.runtime.inc.
There is no user with UUID efb16490-9e30-4466-83fe-0731f29bd769. in services_error() (line 359 of test_site/profiles/wetkit/modules/contrib/services/includes/services.runtime.inc).

however if you are using version 7.x-1.0-alpha5+17-dev as we currently are then you should use a patch found in the wetkit distro queue.

So according to my initial tests it looks like it's safe for us in the wetkit distro to upgrade to a newer dev build of uuid.

Thanks again @Skwashd , I have been testing recent dev versions of UUID for regressions related to this issue and have not found any regressions related to this issue.

so far so good.