Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof commentedDec 20, 2021
would be great to add a test to prevent regressions |
ThomasLandauer commentedDec 20, 2021
Hm, since the header type is passed as string... $header =newUnstructuredHeader('Subject','Test'); ...how can a regression be tested? |
stof commentedDec 20, 2021
@ThomasLandauer create an Email with a |
ThomasLandauer commentedDec 20, 2021
OK, thanks - like this? |
src/Symfony/Component/Mime/Tests/Header/UnstructuredHeaderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Mime/Tests/Header/UnstructuredHeaderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Mime/Tests/Header/UnstructuredHeaderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
stof commentedDec 21, 2021
@nicolas-grekas@fabpot based on the issue being solved, I suggest we consider this as a bugfix. |
nicolas-grekas commentedDec 21, 2021
Works for me |
fabpot commentedDec 21, 2021
Thank you@ThomasLandauer. |
micheh commentedDec 31, 2021
@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.'>'); |
nicolas-grekas commentedDec 31, 2021
Please open issues if you want further discussion. |
ThomasLandauer commentedJan 18, 2022
Yeah, you're right, this might be a BC break - sorry about that! |
micheh commentedJan 19, 2022
Yes, you can use the more generic |
…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
@nicolas-grekas
UnstructuredHeader?IdentificationHeaderTestare 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?