Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Keep body size limit for AMP redirects#62315

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@villermen
Copy link
Contributor

@villermenvillermen commentedNov 5, 2025•
edited by nicolas-grekas
Loading

QA
Branch?6.4
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#62314
LicenseMIT

Copies over body size limit when following redirects inAmpHttpClient. See#62314

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbotcarsonbot changed the titleKeep body size limit for AMP redirects Keep body size limit for AMP redirectsNov 5, 2025
@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.3" but it seems your PR description refers to branch "7.3 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@stof
Copy link
Member

stof commentedNov 5, 2025

The fix looks good to me, but we need to apply it in the 6.4 branch as that branch already disables the body size limit.

villermen reacted with eyes emoji

@villermenvillermenforce-pushed theamp-fix-body-size-limit-on-redirect branch from5909b16 to7f3e591CompareNovember 5, 2025 17:15
@villermenvillermen changed the base branch from7.3 to6.4November 5, 2025 17:15
@villermen
Copy link
ContributorAuthor

@stof Thanks for the pointer. I changed the base to 6.4 now. Note that this version still included just the one version ofAmpResponse. Should I open a second PR to apply it to the splitAmpResponseV4 andAmpResponseV5 in 7.3 as well?

@stof
Copy link
Member

stof commentedNov 5, 2025•
edited
Loading

yes please.

Edit: might not be worth it depending on how the merge-up will go.

@nicolas-grekas
Copy link
Member

Thank you@villermen.

@nicolas-grekasnicolas-grekas merged commit40c2622 intosymfony:6.4Nov 5, 2025
$request->setTcpConnectTimeout($originRequest->getTcpConnectTimeout());
$request->setTlsHandshakeTimeout($originRequest->getTlsHandshakeTimeout());
$request->setTransferTimeout($originRequest->getTransferTimeout());
$request->setBodySizeLimit($originRequest->getBodySizeLimit());

Choose a reason for hiding this comment

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

FTR I changed it this way after merging:

-            $request->setBodySizeLimit($originRequest->getBodySizeLimit());+            $request->setBodySizeLimit(0);+            if (method_exists($request, 'setInactivityTimeout')) {+                $request->setInactivityTimeout(0);+            }

villermen reacted with rocket emoji
@nicolas-grekas
Copy link
Member

Now merged into 7.3

villermen reacted with heart emoji

This was referencedNov 6, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

4 participants

@villermen@carsonbot@stof@nicolas-grekas
[8]ページ先頭
©2009-2025 Movatter.jp