Skip to content

Conversation

joshrencher
Copy link
Contributor

As is, deleteWithObjects() will fail if container is EMPTY. This is because an object count of zero means $secondsToWait = 0, which means $endTime = time(), which means WHILE loop never runs. Quickest solution was to change "time() < $endTime" to "time() <= $endTime" on line 192, but it makes more sense to just delete the container if it's empty.

As is, deleteWithObjects() will fail if container is EMPTY. This is because an object count of zero means $secondsToWait = 0, which means $endTime = time(), which means WHILE loop never runs. Quickest solution was to change "time() < $endTime" to "time() <= $endTime, but it made more sense to just delete the container if it's empty.
@joshrencher joshrencher changed the title Update Container.php make deleteWithObjects() work with empty containers Dec 6, 2014
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than this if-else construction, you can just return early from inside the if. So something like this:

if ($numObjects === 0) {
    return $this->delete();
}

// If timeout (seconds to wait) is not specified by caller, try to
// estimate it based on number of objects in container
// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even awesomer!

On Sun, 07 Dec 2014 14:28:28 -0800, Shaunak Kashyap
[email protected] wrote:

In lib/OpenCloud/ObjectStore/Resource/Container.php:

  • // Attempt to delete all objects and container
  • $endTime = time() + $secondsToWait;
  • $containerDeleted = false;
  • while ((time() < $endTime) && !$containerDeleted) {
  • $this->deleteAllObjects();
  • try {
  • $response = $this->delete();
  • $containerDeleted = true;
  • } catch (ContainerException $e) {
  • // Ignore exception and try again
  • } catch (ClientErrorResponseException $e) {
  • if ($e->getResponse()->getStatusCode() == 404) {
  • // Container has been deleted
  • // If container is empty, just delete it
  • $numObjects = (int)
    $this->retrieveMetadata()->getProperty('Object-Count');
  • if ($numObjects === 0) $response = $this->delete();

Rather than this if-else construction, you can just return early from
inside the if. So something like this:

if ($numObjects === 0) {
return $this->delete();
}

// If timeout (seconds to wait) is not specified by caller, try to
// estimate it based on number of objects in container
// ...

Reply to this email directly or view it on GitHub [1].

Links:

[1]
https://github.com/rackspace/php-opencloud/pull/490/files#r21429173

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, once you send in that fix to this PR, the build should pass on Travis CI and we should be good to merge after that!

1) use getObjectCount() to check container size, 2) return early if empty container deleted, 3) conditional syntax changed for PSR-2 compliance, and 4) removed "and all its objects" from container exception message since we can assume objects in container were successfully deleted by deleteAllObjects();
@ycombinator
Copy link
Contributor

I'd like to make a couple of minor tweaks to this patch, including the necessary changes for the build to pass. I'm going to accept this PR in its current state and make the changes as part of a new PR.

ycombinator added a commit that referenced this pull request Dec 9, 2014
make deleteWithObjects() work with empty containers
@ycombinator ycombinator merged commit 0888361 into rackspace:working Dec 9, 2014
@ycombinator ycombinator mentioned this pull request Dec 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants