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

[Mime] Relaxing in-reply-to header validation#44732

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

Merged
fabpot merged 1 commit intosymfony:4.4fromThomasLandauer:in-reply-to-header
Dec 21, 2021

Conversation

@ThomasLandauer
Copy link
Contributor

QA
Branch?6.1
Bug fix?no
New feature??
Deprecations?no
TicketsFix#37361
LicenseMIT
Doc PRnot necessary

@nicolas-grekas

  1. Is it OK to just useUnstructuredHeader?
  2. Some tests atIdentificationHeaderTest are irrelevant now (but still pass) - should I remove them? And create some new test cases inUnstructuredHeaderTest? Or rely on every aspect being tested with other headers there, and don't add anything?

@stof
Copy link
Member

would be great to add a test to prevent regressions

@ThomasLandauer
Copy link
ContributorAuthor

Hm, since the header type is passed as string...

$header =newUnstructuredHeader('Subject','Test');

...how can a regression be tested?
Create a new test for the entries inprivate const HEADER_CLASS_MAP?

@stof
Copy link
Member

@ThomasLandauer create an Email with aIn-Reply-To header (or aReferences header for the other test to add) being a value that does not respect themsg-id spec, i.e. exactly what fails with the current code of the component.

@ThomasLandauer
Copy link
ContributorAuthor

OK, thanks - like this?

@stof
Copy link
Member

@nicolas-grekas@fabpot based on the issue being solved, I suggest we consider this as a bugfix.

@nicolas-grekas
Copy link
Member

Works for me

@stofstof modified the milestones:6.1,4.4Dec 21, 2021
@ThomasLandauerThomasLandauer changed the title[Mime] Relexing in-reply-to header validation[Mime] Relaxing in-reply-to header validationDec 21, 2021
@fabpot
Copy link
Member

Thank you@ThomasLandauer.

@fabpotfabpot merged commit3526a32 intosymfony:4.4Dec 21, 2021
@ThomasLandauerThomasLandauer deleted the in-reply-to-header branchDecember 21, 2021 23:11
This was referencedDec 29, 2021
@micheh
Copy link
Contributor

@nicolas-grekas Isn't this a BC break? The userland code has to be changed when updating from 5.4.1 to 5.4.2 (use the text header and add the brackets yourself).

Before:

$headers->addIdHeader('References',$messageIds);$headers->addIdHeader('In-Reply-To',$messageId);

After:

$headers->addTextHeader('References','<' .implode('> <',$messageIds) .'>');$headers->addTextHeader('In-Reply-To','<' .$messageId.'>');
AlbinoDrought reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Please open issues if you want further discussion.

@ThomasLandauer
Copy link
ContributorAuthor

Yeah, you're right, this might be a BC break - sorry about that!
You can solve it by changing->addIdHeader() to the generic->addHeader()

@micheh
Copy link
Contributor

Yes, you can use the more genericaddHeader method. But then you also have to remember to add the brackets to each message id yourself, as they are only added with the IdentificationHeader and not with the UnstructuredHeader.

fabpot added a commit that referenced this pull requestAug 10, 2022
…o' and 'References' headers (AlbinoDrought)This PR was merged into the 6.2 branch.Discussion----------[Mime] Re-allow addIdHeader to be used for 'In-Reply-To' and 'References' headers| Q             | A| ------------- | ---| Branch?       | 6.2| Bug fix?      | no (arguable: undo BC break)| New feature?  | yes (arguable: undo BC break)| Deprecations? | no| Tickets       |Fix#45097| License       | MIT| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->#44732 causes messages using `addIdHeader('In-Reply-To', ...)` or `addIdHeader('References', ...)` to fail the headers check (see#44732 (comment) ), requiring users to manually format the header value themselves.This PR re-allows the previous behaviour of `addIdHeader` while keeping the new unstructured behaviour in-place as the default.-----(I'm not sure if this counts as a new feature or a bug fix since it "reintroduces" a previous feature)Commits-------ffde0f1 [Mime] Re-allow addIdHeader to be used for 'In-Reply-To' and 'References' headers
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@stofstofstof approved these changes

@chalasrchalasrAwaiting requested review from chalasr

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@wouterjwouterjAwaiting requested review from wouterj

@xabbuhxabbuhAwaiting requested review from xabbuh

@ycerutoycerutoAwaiting requested review from yceruto

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

[Mailer] Too strict syntax validation forIn-Reply-To andReferences headers

6 participants

@ThomasLandauer@stof@nicolas-grekas@fabpot@micheh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp