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

Start work on the WorkersKV API so we can write and read info from the KV system #139

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

alnutile
Copy link

This is work to start making this part of the PHP library.

Not all the API endpoints are there. I will add more shortly.

@@ -0,0 +1,22 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

looks like this file should have been ignored from the gitignore above. can we remove it?

@@ -1,3002 +0,0 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

did you forget to commit these changes? looks like the entire file has been deleted and not updated.

Comment on lines +3 to +8
/**
* User: junade
* Date: 01/02/2017
* Time: 12:30
*/

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* User: junade
* Date: 01/02/2017
* Time: 12:30
*/

public function writeMultipleKeyValuePairs(string $accountID, $namespaceIdentifier, $keys = []): bool
{
$this->adapter->put('accounts/' . $accountID . '/storage/kv/namespaces/' . $namespaceIdentifier . '/bulk', $keys);
return true;
Copy link
Member

Choose a reason for hiding this comment

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

do we want to handle the exception here if the PUT request fails? i.e. additional logging, output, etc?

$this->assertEquals("Some Value", $result);
}

public function testDeleteKeyValuePair ()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function testDeleteKeyValuePair ()
public function testDeleteKeyValuePair()

public function getReadKeyValuePair(string $accountID, $namespaceIdentifier, $keyName): string
{
$response = $this->adapter->get('accounts/' . $accountID . '/storage/kv/namespaces/' . $namespaceIdentifier . '/values/' . $keyName);
$this->body = json_decode($response->getBody());
Copy link
Contributor

@deuill deuill Mar 9, 2021

Choose a reason for hiding this comment

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

My understanding is that the Content-Type for these responses is application/octet-stream, so we shouldn't be json_decode-ing here. Since the type returned by $response->getBody(), StreamInterface, supports __toString() and seems to do the right thing, it might be worth changing this to:

-        $this->body = json_decode($response->getBody());
+        $this->body = (string) $response->getBody();

@thecodeassassin
Copy link

hello @alnutile @deuill Anything I can do to help get this merged? We are now in the process of writing a custom library for this.

@rennokki
Copy link

rennokki commented Jan 11, 2022

If someone is not taking the action to fix the changes request I'll fork it by myself and submit the PR as it should be 😂


public function deleteKeyValuePair(string $accountID, $namespaceIdentifier, string $key): bool
{
$this->adapter->delete('accounts/' . $accountID . '/storage/kv/namespaces/' . $namespaceIdentifier . '/values', [$key]);

Choose a reason for hiding this comment

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

I've just had to use this method and it looks like this might be an old way of deleting the KV pair as it is now throwing a 404 error.
According to the documentation the key should be part of the url instead of the body:
https://api.cloudflare.com/#workers-kv-namespace-delete-key-value-pair

Suggested change
$this->adapter->delete('accounts/' . $accountID . '/storage/kv/namespaces/' . $namespaceIdentifier . '/values', [$key]);
$this->adapter->delete('accounts/' . $accountID . '/storage/kv/namespaces/' . $namespaceIdentifier . '/values/' . $key);

@cnizzardini
Copy link

Any plans to finish this?

@prionkor
Copy link

is this repo fully community driven or CF employees here to help? KV support is needed badly!

@alnutile
Copy link
Author

Sorry just noticed the comments :( lost track of the project since the client did not need it.

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.

9 participants