feat: doesObjectExist and doesBucketExist v2 #2424
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SamRemis
reviewed
Apr 12, 2022
SamRemis
reviewed
Apr 12, 2022
SamRemis
reviewed
Apr 15, 2022
src/S3/S3ClientTrait.php
Outdated
| $this->execute($command); | ||
| return true; | ||
| } catch (S3Exception $e) { | ||
| if (($accept403 && $e->getStatusCode() === 403) |
There was a problem hiding this comment.
there should be a newline after the first parenthesis
src/S3/S3ClientTrait.php
Outdated
| $response = $e->getResponse(); | ||
|
|
||
| if ($includeDeleteMarkers | ||
| && (!empty($response) |
There was a problem hiding this comment.
Maybe extract this into a hasDeleteMarker method that checks if a response exists and returns whether or not that header is there? Condense a a bit of code.
There was a problem hiding this comment.
totally agree. just took a second glance and that looks pretty ugly
added 2 commits
April 15, 2022 17:57
SamRemis
approved these changes
Apr 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Implements #1561, closes #2342
Description of changes:
New object/bucket existence helper methods with changed exception handling behavior. Previously,
falsewas returned any time an exception was thrown by the underlying S3 APIs (headObjectandheadBucket) unless the exception's status code was >=500(then exception was thrown) or the exception's error code wasAccessDenied(truewas returned). This was problematic becausefalsecould be returned in the cases of an unresolvable endpoint, expired credentials, invalid credentials, or if bucket permissions were not granted to the calling entity and resources did in fact exist.This implementation makes fewer assumptions in cases where status codes are ambiguous, namely
403status codes. a403status code can signify bad credentials or incorrect bucket-level permissions— this causes issues withheadObject/doesObjectExistcalls in particular because a 403 will be returned whether or not an object exists. In this implementation, exceptions will be thrown in all cases when an exception is thrown by the underlying API, unless the exception has a404status code (returns false) or in the case ofDoesBucketExist, a user can specify if they want to accept403status codes, as a 403 will be thrown if a bucket exists, but an entity doesn't have correct bucket-level permissions.Other changes include accepting bucket redirects (incorrectly specified region) as a
truecase forDoesBucketExist, the$accept403flag fordoesBucketExistand the$includeDeleteMarkersflag fordoesObjectExist, which counts a404response with the presence of a delete marker header as atruecase.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.