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

Appearance settings

[HttpFoundation] Rename Request::getContentType to getContentTypeFormat #45034

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

Merged
merged 1 commit into from
Jul 22, 2022

Conversation

MarkPedron
Copy link
Contributor

@MarkPedron MarkPedron commented Jan 15, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Fix #39750
License MIT
Doc PR TODO

@carsonbot

This comment was marked as resolved.

@MarkPedron MarkPedron force-pushed the issue_39750_request_get_content_type branch from efea1a1 to f4409c8 Compare January 15, 2022 18:55
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Looks good from my point of view 👍

src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
@MarkPedron MarkPedron force-pushed the issue_39750_request_get_content_type branch 4 times, most recently from 59db8a5 to c2c3b20 Compare January 15, 2022 19:30
@MarkPedron
Copy link
Contributor Author

@wouterj Thank you for your kind feedback.

Sorry for all the updates, I completely forgot to add tests and the necessary deprecation infos.

Should be ready now. The CI failure is very mysterious to me.

@wouterj
Copy link
Member

wouterj commented Jan 15, 2022

No problem! For a first contribution, this is in perfect shape (especially considering changes like this need BC and deprecations and stuff).

@MarkPedron MarkPedron requested a review from chalasr as a code owner January 15, 2022 19:52
@MarkPedron
Copy link
Contributor Author

MarkPedron commented Jan 15, 2022

I made an attempt to make appveyor happy (I think i understood the subsequent deprecation emitted in security/http).
The errors in the php8.0/php8.1 job also seem understandable (the return type patch does not apply, because the code changed). However, they seem unrelated to this PR/issue.

@MarkPedron MarkPedron force-pushed the issue_39750_request_get_content_type branch from f949c8c to 75daa15 Compare January 15, 2022 20:00
$request = new Request();
$contentType = $request->getContentTypeFormat();

$this->assertNull($contentType);
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to add more tests covering cases where that method returns a non-null value. Otherwise, we don't really have tests for the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. I added tests to cover the mime types application/json and text/html

src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
@carsonbot carsonbot changed the title Rename Request::getContentType to getContentTypeFormat [HttpFoundation] Rename Request::getContentType to getContentTypeFormat Apr 19, 2022
@nicolas-grekas
Copy link
Member

Friendly ping @MarkPedron, up to finish this PR? See pending comments + rebase needed.

@MarkPedron MarkPedron force-pushed the issue_39750_request_get_content_type branch 2 times, most recently from 6b0c8e2 to c092b30 Compare April 26, 2022 19:45
@MarkPedron
Copy link
Contributor Author

@nicolas-grekas Thanks for the reminder. I added non-null tests as suggested, accepted the suggestion expanding the comment, and rebased. If there is anything else to be done, please let me know. I am hesitating to expand the comments further, as to me the meaning of getContentTypeFormat seems quite clear, and confusion arises from the methods setRequestFormat/getRequestFormat, which (to my understanding) refer to the negotiated format of the Response.

UPGRADE-6.1.md Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
@fabpot fabpot force-pushed the issue_39750_request_get_content_type branch from c092b30 to ccc77fc Compare July 22, 2022 12:34
Resolves issue symfony#39750.

The method getContentType was confusing.
This method does not return a mime type, but a mapped type name derived
from the mime type in the CONTENT_TYPE header.
@fabpot fabpot force-pushed the issue_39750_request_get_content_type branch from ccc77fc to f545ed4 Compare July 22, 2022 12:37
@fabpot
Copy link
Member

fabpot commented Jul 22, 2022

Thank you @MarkPedron.

@fabpot fabpot merged commit 57b24fe into symfony:6.2 Jul 22, 2022
@fabpot fabpot mentioned this pull request Oct 24, 2022
nicolas-grekas added a commit that referenced this pull request Jun 30, 2023
…mNaN)

This PR was merged into the 6.4 branch.

Discussion
----------

[Security] Remove BC layer for HttpFoundation < 6.2

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | n/a

In #45034, a condition was added to support `symfony/http-foundation < 6.2` or use the new `Request::getContentTypeFormat()` method when it exists.

I propose to update the minimum version of `symfony/http-foundation` required by `symfony/security-http: 6.4`.
`symfony/security-http: 6.4` already requires `symfony/http-kernel: ^6.3|^7.0` [which require](https://github.com/symfony/symfony/blob/6.3/src/Symfony/Component/HttpKernel/composer.json) `symfony/http-foundation: ^6.2.7`

The legacy method `Request::getContentType()` will be removed in 7.0 by #50826

Commits
-------

746c3fd Remove BC layer for HttpFoundation < 6.1
nicolas-grekas added a commit that referenced this pull request Jun 30, 2023
… behaviors (GromNaN)

This PR was squashed before being merged into the 7.0 branch.

Discussion
----------

[HttpFoundation] Remove deprecated classes, method and behaviors

| Q             | A
| ------------- | ---
| Branch?       | 7.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | n/a

Clean `symfony/http-foundation` from all its legacy.

- Remove `RequestMatcher` and `ExpressionRequestMatcher`,
  deprecated since #47595
- Remove `Request::getContentType()`,
  deprecated since #45034
- Throw a `UnexpectedValueException` or `BadRequestException` when `ParameterBag::filter()` or `InputBag::filter()` reads an invalid value and the flag `FILTER_NULL_ON_FAILURE` is not set.
  new behavior announced since #48525
- Throw a `InvalidArgumentException` when calling `Request::create()` with a malformed URI,
  deprecated since #49376

Commits
-------

665a775 [HttpFoundation] Remove deprecated classes, method and behaviors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request::getContentType() method name is confusing
9 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.