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

Fix email subject with special characters (issue 1433)](https://github.com/wielebenwir/commonsbooking/pull/1479 #1480

Merged
merged 8 commits into from
Mar 17, 2024

Conversation

nelarsen
Copy link
Contributor

@nelarsen nelarsen commented Dec 28, 2023

make commonsbooking_parse_template() customizable with respect to sanitation to allow for HTML-sanitation (default) or email subject (sanitize_text_field)-sanitation. Should fix issue 1433 for e-mail subjects with special characters like &

This solution simlar to the fix for the first part of issue 1433 (the from field) where we made get_option configurable with respect to sanitation function. I prefer the alternative variant, 1 though. I dislike the long argument lists to be passed deep inside library functions.

EDIT @datengraben: Initially this was related to this issue #1433 which was fixed in #1426. But there is no separate issue for the subject problem handled with this PR here.

Alternative implementation as (first) variant of this branch was proposed in #1479 but closed in favor of this pr.

…itazion to allow for HTML-sanitazion (default) or email subject (sanitize_text_field). Should fix issue 1433 for e-mail subjects with special characters like &
Copy link

codecov bot commented Dec 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a94e930) 41.27% compared to head (ae2f9dd) 41.27%.
Report is 47 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1480   +/-   ##
=========================================
  Coverage     41.27%   41.27%           
  Complexity     2329     2329           
=========================================
  Files            91       91           
  Lines          9613     9613           
=========================================
  Hits           3968     3968           
  Misses         5645     5645           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hansmorb
Copy link
Contributor

Ich glaube ich finde diese Variante besser, weil sie die Verwendung einer sanitization Funktion für jeden Input Wert vorraussetzt.

@hansmorb hansmorb requested a review from chriwen December 29, 2023 04:27
*
* @return false|mixed
*/
function commonsbooking_parse_template_callback( $match, array $objects = [] ) {
function commonsbooking_parse_template_callback( $match, array $objects = [], $sanitizeFunction ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Kannst du den default Wert hier auf commonsbooking_sanitizeHTML setzen? Dann kann die Funktion auch ohne default Wert ausgeführt werden und ist sehr ähnlich zu der ursprünglichen Funktion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gerne, habe ich eben gemacht und einmal getestet.

@hansmorb hansmorb marked this pull request as ready for review January 4, 2024 17:17
@hansmorb
Copy link
Contributor

hansmorb commented Jan 4, 2024

Hab mal die Unit Tests gemergt, von der Seite aus funktioniert es aber ich habe jetzt das Problem, dass mir teilweise das Subject gar nicht mehr angezeigt wird:

image

Zum Reproduzieren einfach in der englischen Installation mal den Subject Wert löschen (das stellt den Default Wert wieder her)

@nelarsen
Copy link
Contributor Author

nelarsen commented Jan 6, 2024

Hallo Hans, ich verstehe nicht ganz, was bei dir "nicht mehr angezeigt wird". In deinem Screenshot gibt es ein m.E. gültiges Subject-Feld (Quoted-Printable-Kodierung). Ich habe die WP-Sprache auf Englisch umgestellt und den Default-Wert der Betreffvorlage wiederhergestellt. Dann kommt das an (Rohansicht bzw. Bildschirmabdruck in Thunderbird):

xxxx
To: xxxxx
Subject: =?us-ascii?Q?Your_booking_ADFC_Henry_&_Wilde_Hilde_at_Testst?=
 =?us-ascii?Q?andort_B_on_15._January_2024?=
Date: Sat, 6 Jan 2024 14:12:24 +0000
From: Frieda & Friedrich-Buchungssystem <fragen@friedafriedrich.de>
X-Mailer: PHPMailer 6.8.1 (https://github.com/PHPMailer/PHPMailer)
MIME-Version: 1.0
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

grafik

Allerdings: das ist mit dem letzten Commit von mir. Deine Version (master gemerged) kann ich gerne testen, wenn du mir eine kompilierte ZIP-Datei schickst. Leider weiß ich nicht, wie ich auf Ubuntu commonsbooking selbst kompiliere. Ist das irgendwo vollständig beschrieben?

@hansmorb
Copy link
Contributor

hansmorb commented Jan 7, 2024

Huhu, kannst du nicht einfach auf deinem Branch git pull ausführen? Ansonsten kannst du eine zip erstellen, indem du in dem Verzeichnis die bin/build-zip.sh aufrufst. Du kannst aber vorher nochmal sicherstellen, dass auch wirklich alle dependencies installiert sind indem du npm run start ausführst

@nelarsen
Copy link
Contributor Author

nelarsen commented Jan 8, 2024

Cool, bin/build-zip.sh hat für mich zum ersten Mal funktioniert, danke für den Tipp. Ich weiß nicht, was ich bisher falsch gemacht habe. Ich habe jetzt den aktuellen fix_email_subject Branch getestet und komme zu keinem anderen Ergebnis als am 6. Januar. Ich habe nicht verstanden, was bei dir nicht funktioniert (siehe meinen "Einwand" vom 6. Januar oben)?

@hansmorb
Copy link
Contributor

Das Problem ist, dass ich zum Testen Local by Flywheel nutze und mit Mailhog die E-Maisl abfange, die die Instanz erzeugt. Dabei wird, wenn dieser seltsame Unicode Character drin ist, die Betreffzeile nicht mehr angezeigt. Ich kann jetzt nur leider nicht richtig nachstellen wie das bei einer Thunderbird E-Mail aussehen würde. Aber du meinst, dass die Betreffzeile bei dir noch funktioniert obwohl dieses Sonderzeichen vorhanden ist?

@nelarsen
Copy link
Contributor Author

Hallo Hans, ja, bei mir wird die Betreffzeile korrekt in Thunderbird angezeigt, wenn der Subject header diesen Kauderwelsch (Quoted-Printable) enthält: "Subject: =?us-ascii?Q?Your_booking_ADFC_Henry_&_Wilde_Hilde_at_Testst?=
=?us-ascii?Q?andort_B_on_15._January_2024?=" (siehe meine Antwort vom 6.1.2024)
Was müsste ich tun, um mit Local und Mailhog nachzuprüfen? Es ist nicht auszuschließen, dass Mailhog kaputt ist, siehe hier. Da geht es zwar um ein anderes, ähnlich gelagertes Problem (das seit 2015 besteht und nie gelöst wurde) und ganz unten heißt es "Mailhog is not actively maintained, I recommend switching to https://github.com/axllent/mailpit"

@datengraben
Copy link
Contributor

datengraben commented Feb 15, 2024

@nelarsen @hansmorb dieser PR ist schon gemerged #1426 aber dieser hier ist auch noch relevant oder? Zwecks Release räume ich gerade auf und würde diesen dann in den 2.9.1 schieben.

@datengraben datengraben added the bug Something isn't working label Feb 17, 2024
@datengraben datengraben added this to the 2.9.1 milestone Feb 17, 2024
@datengraben datengraben changed the title Fix (variant 2) for special characters in e-mail subject (issue 1433)](https://github.com/wielebenwir/commonsbooking/pull/1479 Fix email subject with special characters (issue 1433)](https://github.com/wielebenwir/commonsbooking/pull/1479 Feb 17, 2024
@hansmorb hansmorb removed this from the 2.9.1 milestone Mar 3, 2024
@hansmorb hansmorb added this to the 2.9.2 milestone Mar 14, 2024
@hansmorb hansmorb linked an issue Mar 17, 2024 that may be closed by this pull request
Copy link
Contributor

@hansmorb hansmorb left a comment

Choose a reason for hiding this comment

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

Wenn das bei dir funktioniert hat dann wird das wohl eher an Mailhog liegen, dann können wir das so gerne übernehmen.

@hansmorb hansmorb merged commit 3185b24 into wielebenwir:master Mar 17, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E-Mail-Absender Sonderzeichen falsch dargestellt
3 participants