Skip to content

Conversation

alexgt9
Copy link

@alexgt9 alexgt9 commented Jan 3, 2020

The rename method fails when using non url safe chars in the source parameter.

Error executing "CopyObject" on "https://****.amazonaws.com/FileWithNonUrlSafeChar%_.pdf"; AWS HTTP error: Client error: PUT https://eu-west-1-sg-prod-files.s3.eu-west-1.amazonaws.com/FileWithNonUrlSafeChar%_.pdf resulted in a 400 Bad Request response:
InvalidArgumentInvalid copy source encoding.x-amz-copy-source</Argu (truncated...)

I just url encode the CopySource parameter in the API call

$this->filesystem->write('foo', '');
$this->filesystem->rename('foo', 'foo%_encode');

$this->assertTrue($this->filesystem->has('foo'));
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the original name not be present ?

Copy link
Author

Choose a reason for hiding this comment

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

You are right 👌
Anyway there is no environment where this tests for S3 are executed?

@alexgt9 alexgt9 changed the title Add test for rename url not safe source S3 Fix rename method for url not safe source Jan 7, 2020
@alexgt9 alexgt9 changed the title S3 Fix rename method for url not safe source AwsS3 Fix rename method for url not safe source Jan 7, 2020
Copy link
Contributor

@Nek- Nek- left a comment

Choose a reason for hiding this comment

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

Hello, thank you very much for trying to improve Gaufrette.

I didn't test your fix but I see that the test passed without the change you did in the code.

/**
* @test
*/
public function shouldRenameAnObject()
Copy link
Contributor

Choose a reason for hiding this comment

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

This test pass with and without the fix. The test name is also not clear regarding what it fixes.

@PedroTroller PedroTroller self-assigned this Apr 5, 2023
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.

4 participants