andig / carddav2fb

Download CardDAV VCards and upload as phonebook to AVM FRITZ!Box
63 stars 19 forks source link

Clean up quote backslashes before some special chars. Fixes #90 #91

Closed derwok closed 5 years ago

derwok commented 5 years ago

Fixes #90

andig commented 5 years ago

Mhhm. Also wenn CardDav das falsch schickt dann gehört das in den Carddav Parser und nicht in den Upload?

derwok commented 5 years ago

Ja - hab versucht eine geeignete Stelle früher in der Kette zu finden...

Aber im Skript fassen wir ja mehrere Sub-Attribute zum "realName" zusammen. Und der Converter Code mit dem Tokens war für mich leider nicht direkt verständlich.

Ich finde daher diesen späten Qualitäts-Sicherer soweit auch prima. :-) Falls Du es woanders lieber hättest, bräuchte ich einen konkreten Vorschlag (Datei/Zeile).

andig commented 5 years ago

Irgendwo hier: https://github.com/andig/carddav2fb/blob/master/src/CardDav/Backend.php mangels carddav Server fehlt mir da dir konkrete Idee.

andig commented 5 years ago

Könnte vielleicht auch hier hin passen: https://github.com/andig/carddav2fb/blob/master/src/Vcard/Parser.php#L330

derwok commented 5 years ago

Ist nicht so, dass ich deinen Punkt nicht verstehe. Aber ich bin der Meinung, wir sollten den genau dort lassen.

Denn wir haben ja einen sehr flexiblen Converter, der den in der Fritzbox angezeigten RealNamen aus beliebigen(!) Teil-Attributen der Original-vCard zusammensetzen kann (siehe config.php conversions/realName).

Wenn das Ziel ist: "Es soll in der FritzBox gut aussehen", dann müssten wir auf Verdacht alle(!) vCard-Attribute dermaßen behandeln. Auch die, die wir noch nicht kennen. Aber auf keinen Fall Telefnonnumern oder EMail-Adressen! Das frühe Patchen klingt daher für mich komplex und gefährlich.

Oder halt - wie derzeit im PR - einfach einmal den realName-String der in der Fritzbox zur Anzeige kommt... Genau das ist war ja das Ziel. Dabei kann es uns dann egal sein, wie der realName mal zusammengesetz wurde.

Unter den gegebenen Randbedingungen finde ich daher die Lösung akzeptabel.

andig commented 5 years ago

Denn wir haben ja einen sehr flexiblen Converter, der den in der Fritzbox angezeigten RealNamen aus beliebigen(!) Teil-Attributen der Original-vCard zusammensetzen kann (siehe config.php conversions/realName).

Das stimmt. Aber das Problem ist ja nicht das Zusammensetzen, sondern die schon falschen Einzelteile. Oder versteh ich das falsch?

Wenn das Ziel ist: "Es soll in der FritzBox gut aussehen", dann müssten wir auf Verdacht alle(!) vCard-Attribute dermaßen behandeln.

Ja, oder eben da wo unescape zum Einsatz zum Einsatz kommt.

Auch die, die wir noch nicht kennen. Aber auf keinen Fall Telefnonnumern oder EMail-Adressen!

Warum? Gibts da legale Anwendungen für willenloses \,?

Das frühe Patchen klingt daher für mich komplex und gefährlich.

Versteh ich, aber wir frickeln hier an der falschen Stelle- das holt einen später immer wieder ein :/

derwok commented 5 years ago

Könnte vielleicht auch hier hin passen: https://github.com/andig/carddav2fb/blob/master/src/Vcard/Parser.php#L330

Hmmm... das wird leider nur bei der Konvertierung einer vCard "NOTE" verwendet. Und die ignorieren wir im Skript.

derwok commented 5 years ago

Auch die, die wir noch nicht kennen. Aber auf keinen Fall Telefnonnumern oder EMail-Adressen!

Warum? Gibts da legale Anwendungen für willenloses \,?

OK. Hast Recht! Auch in Telefonnummern kommen die "COMMA" falsch raus. Wir müssen auch dort die BACKSLASH entfernen. Denn Komma in Telefonnummern sind erlaubt.

Ich hab jetzt auch die Referenz, dass die Backslashes per vCard Standard gewollt sind: https://tools.ietf.org/html/rfc6350#section-3.4

Ich bastel es in den Parser rein.

andig commented 5 years ago

I've opened an issue at the underlying library at https://github.com/jeroendesloovere/vcard/issues/138. We're using a copy of the code as it could not be extended for custom elements.

andig commented 5 years ago

Ich versteh Deine Monster Regex nicht. Willst Du nicht einfach:

'#\([,;\])#'

und dann mit

'\$1' 

ersetzen? Bei ' entfällt das Aliasing und Escaping in PHP ggü ". Irgendwo anders hatten wir das glaub ich auch schon?

Update aktualisiert- betrifft ja ,;\

derwok commented 5 years ago

Ich fand es dann doch so einfacher zu lesen...

derwok commented 5 years ago

Hab die 4 commits ge-squashed.

andig commented 5 years ago

Hab die 4 commits ge-squashed.

Musst Du nicht, das geht in github beim merge.