Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delete object method in the container of ObjectStore. #651

Merged
merged 3 commits into from
Jan 29, 2016
Merged

Add delete object method in the container of ObjectStore. #651

merged 3 commits into from
Jan 29, 2016

Conversation

Ducatel
Copy link
Contributor

@Ducatel Ducatel commented Jan 15, 2016

Hi everybody,

I add deleteObject method in the container class of object storage API.
This method is faster than the previous way to delete an object from the object storage.

According to the documentation, previously you must download the object and after delete it.
The first step isn't always relevant (Can generate some performance issue if the object is big).

$container = $objectStoreService->getContainer('{containerName}');
$object = $container->getObject('{objectName}'); // Can be long for big file
$object->delete();

Now you can delete it without downloaded it.

$container = $objectStoreService->getContainer('{containerName}');
$container->deleteObject('{objectName}');

Have fun ;)

@jamiehannaford
Copy link
Contributor

@Ducatel This is a way around this, but it's not well documented:

$container = $objectStoreService->getContainer('{containerName}');

$object = $container->object();
$object->setName('{objectName}');
$object->delete();

Your way is easier though

@Ducatel
Copy link
Contributor Author

Ducatel commented Jan 15, 2016

@jamiehannaford Your way can be added in the documentation.
I'm also agree with you but for me your way aren't "really natural".

EDIT:
In fact, I tried your way but

No method OpenCloud\ObjectStore\Resource\Container::object()

I find another way with

$container = $objectStoreService->getContainer('{containerName}');
$partialObject = $container->getPartialObject('objectName'); // Faster than getObject
$partialObject->delete();

EDIT 2:

Ok, I found what you explain. It's also working ;)

$container = $objectStoreService->getContainer('{containerName}');
$object = new \OpenCloud\ObjectStore\Resource\DataObject($container);
$object->setName('{objectName}');
$object->delete();

@Ducatel
Copy link
Contributor Author

Ducatel commented Jan 26, 2016

Hi,
Just can you tell me if you will merge my pull request ?
If yes, when the next version (which include it) should be available ?

thanks ;)

}
}
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

My problem with this is what happens if a 500 or 403 is returned? All the user will get is false. One option would be to return nothing for success, and an exception for failure. So it could just be:

$this->getClient()
     ->delete($this->getUrl($name))
     ->send();

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it will be more consistent with the rest of API.
If the code change, we need to update unit test of this function.

@jamiehannaford
Copy link
Contributor

Sorry this took so long to get to. I've left 1 comment that requires a bit of feedback before we continue

@Ducatel
Copy link
Contributor Author

Ducatel commented Jan 28, 2016

No problem. I will make the update

@Ducatel
Copy link
Contributor Author

Ducatel commented Jan 29, 2016

My fix seems good for you ?
Just a question, is it possible to add php7 in travisCI test ?

*/
public function deleteObject($name)
{
$response = $this->getClient()
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to have $response = since the variable is never used

@jamiehannaford
Copy link
Contributor

@Ducatel Thanks for pushing these changes. There's just two very minor issues left before we can merge. In terms of PHP 7, I'm pretty sure the build will always break - so not sure there's much point adding it to the matrix. Same with HHVM. If it passes, though, I'm happy to add.

@Ducatel
Copy link
Contributor Author

Ducatel commented Jan 29, 2016

arf, I forgot to delete theses variables, my apologies...

I use PHP7 and HHVM so, I will probably check if all tests are passing on theses environments (and make some fix if necessary)

jamiehannaford pushed a commit that referenced this pull request Jan 29, 2016
Add delete object method in the container of ObjectStore.
@jamiehannaford jamiehannaford merged commit d6b71fe into rackspace:working Jan 29, 2016
@jamiehannaford
Copy link
Contributor

Sounds good 👍 Feel free to submit a PR if PHP 7 or HHVM works

@Ducatel
Copy link
Contributor Author

Ducatel commented Jan 29, 2016

Ok, no problem.
Do you know, when the next version will be release ? (I didn't found the roadmap.)

EDIT: Ok, so the version is released ;)
EDIT2: PHP 7 version produce some error --> https://travis-ci.org/Ducatel/php-opencloud/jobs/105681998#L67

@jamiehannaford
Copy link
Contributor

@Ducatel yep, just now 😄

@jamiehannaford
Copy link
Contributor

You can still submit a PR for the Travis builds and I'll increment to v1.16.1

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.

None yet

2 participants