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

Resolve https://github.com/ruflin/Elastica/issues/1737: handle 413 response #2055

Merged
merged 9 commits into from
Mar 29, 2022

Conversation

Vetaxon
Copy link
Contributor

@Vetaxon Vetaxon commented Feb 15, 2022

Resolve #1737

  • introduced RequestEntityTooLargeException
  • adjusted Bulk, throw RequestEntityTooLargeException if response code 413

- adjusted Bulk, throw RequestEntityTooLargeException if response code 413
Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Could you add a changelog entry?

src/Bulk.php Show resolved Hide resolved
@ruflin
Copy link
Owner

ruflin commented Mar 16, 2022

@Vetaxon Change LGTM. Could you add a changelog entry so we can get this in?

@Vetaxon
Copy link
Contributor Author

Vetaxon commented Mar 21, 2022

@ruflin Hi. I have added updates to change the changelog.md, pls check. Also, when can we expect release of it? We need it for all our projects (more then 50).

Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Around the release, I wonder as this is a bigger change if we should do a 7.2? @thePanz Thoughts?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/Bulk.php Show resolved Hide resolved
@ruflin
Copy link
Owner

ruflin commented Mar 23, 2022

Thanks for the changelog @Vetaxon Seems like you are hitting a conflict there and need to merge in or rebase on master.

Could you also have a look at the comment from @thePanz here? #2055 (comment) Would be great to have a test covering this.

Vetaxon and others added 2 commits March 29, 2022 10:19
added test BulkTest to check RequestEntityTooLargeExceptionTest
@Vetaxon
Copy link
Contributor Author

Vetaxon commented Mar 29, 2022

Hi, thanks for being patient
Conflicts have been solved
Tests have been added
pls check
@thePanz
@ruflin

@Vetaxon Vetaxon requested review from ruflin and thePanz March 29, 2022 08:44
Copy link
Collaborator

@thePanz thePanz left a comment

Choose a reason for hiding this comment

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

Thank you @Vetaxon for the tests, just a minor comment

tests/Exception/RequestEntityTooLargeExceptionTest.php Outdated Show resolved Hide resolved
tests/BulkTest.php Outdated Show resolved Hide resolved
src/Exception/RequestEntityTooLargeException.php Outdated Show resolved Hide resolved
@Vetaxon Vetaxon requested a review from thePanz March 29, 2022 13:44
@Vetaxon Vetaxon requested a review from thePanz March 29, 2022 14:11
tests/BulkTest.php Outdated Show resolved Hide resolved
@Vetaxon
Copy link
Contributor Author

Vetaxon commented Mar 29, 2022

@ruflin Hi, it seems PR is ready! Thanks

@thePanz thePanz merged commit 7723faa into ruflin:master Mar 29, 2022
@thePanz
Copy link
Collaborator

thePanz commented Mar 29, 2022

Merged, thanks @Vetaxon !

@Vetaxon
Copy link
Contributor Author

Vetaxon commented Mar 29, 2022

@thePanz thanks a lot. Is it known when it can be release?

@thePanz
Copy link
Collaborator

thePanz commented Mar 29, 2022

@Vetaxon : 7.1.5 has been released now

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.

Suppression of 413 error code (payload size exceeded)
4 participants