Initial API #2
Initial API #2
Conversation
| $bodymd5 = base64_encode(hex2bin(md5($body))); | ||
| $request = new Request('PUT', $this->build_blocklist_url($key), | ||
| ['Content-Type' => 'application/xml', 'content-md5' => $bodymd5], $body); | ||
| $this->client->sendAsync($request)->then(null, $this->clean_exception_sas_if_needed())->wait(); |
There was a problem hiding this comment.
We are only using sync operations in general. Does is make sense to use the sync API instead?
There was a problem hiding this comment.
We need the async api so we can use ->then to catch the exception and handle it - with the sync api I don't think this is possible without standard try/catch which is a lot messier
| * Returns a request exception handling function that redacts the SAS token from error messages if needed. | ||
| * @return callable | ||
| */ | ||
| private function clean_exception_sas_if_needed(): callable { |
There was a problem hiding this comment.
can we just call this clean_exception_sas_token? seems a bit cleaner, I dont think the if_needed is required
There was a problem hiding this comment.
i mean, it doesn't 100% of the time clean the exception sas, only if the flag/setting is enabled. So technically is is if_needed - while a bit verbose I think its good as it conveys it doesn't do it 100% of the time
classes/stream_wrapper.php
Outdated
| * @param api $client Client to use with the stream wrapper | ||
| * @param string $protocol Protocol to register as. | ||
| */ | ||
| public static function register(api $client, $protocol = 'blob') { |
There was a problem hiding this comment.
Should we ever allow anything OTHER than blob here? This feels like it should be hardcoded rather than a param
There was a problem hiding this comment.
Technically the protocol is a param in the stream wrapper spec.
If you changed to have it be something else it would stop working with objectfs (objectfs would need to have it changed on its side as well) but technically this plugin is not objectfs specific, so I don't see a reason why it needs to be hardcoded.
Changes
Notes
I made the decision to move the stream wrapper here and out of objectfs, since this follows the same pattern that AWS does (the AWS sdk includes a stream wrapper) - nothing about the stream wrapper is objectfs specific.
This also targets 4.2 specifically to align with ObjectFS's existing 4.2 branch, instead of having to create a new corresponding 4.4 objectfs branch.