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

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

Merged

Conversation

derwok
Copy link

@derwok derwok commented Jan 12, 2019

Fixes #90

@andig
Copy link
Owner

andig commented Jan 12, 2019

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

@derwok
Copy link
Author

derwok commented Jan 12, 2019

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
Copy link
Owner

andig commented Jan 12, 2019

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

@andig
Copy link
Owner

andig commented Jan 12, 2019

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

@derwok
Copy link
Author

derwok commented Jan 12, 2019

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
Copy link
Owner

andig commented Jan 12, 2019

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
Copy link
Author

derwok commented Jan 12, 2019

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
Copy link
Author

derwok commented Jan 12, 2019

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
Copy link
Owner

andig commented Jan 12, 2019

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

@andig
Copy link
Owner

andig commented Jan 12, 2019

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 derwok force-pushed the #90-clean-out-XML-quote-backslashes-in-realNames branch from 980e461 to 9a1055b Compare January 12, 2019 20:39
@derwok
Copy link
Author

derwok commented Jan 12, 2019

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

@derwok
Copy link
Author

derwok commented Jan 12, 2019

Hab die 4 commits ge-squashed.

@andig
Copy link
Owner

andig commented Jan 12, 2019

Hab die 4 commits ge-squashed.

Musst Du nicht, das geht in github beim merge.

@andig andig merged commit 3c3730d into andig:master Jan 12, 2019
@derwok derwok deleted the #90-clean-out-XML-quote-backslashes-in-realNames branch January 27, 2019 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants