Hi Clemens,
while investigating Debian bug #837154 ("backslash escaping sometimes
broken in vCard", see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=837154)
I happened upon an AWL commit by you merged about a year ago:
Fixed grouped Properties naming (e.g. Addresses: item1.ADR instead of just
ADR) that caused item1.ADR to be written to DB(address_address_adr) because it
doesn't match ADR, fix works ofr every grouped Property (yet there is only
ADR...).
Added VCard Property ORG as nondefault (because it takes more then one
Value).
Fixed false handling of Properties that can have more than one value (e.g.
ORG) where values are seperated by semicolons
https://gitlab.com/davical-project/awl/commit/164e22151e1ed80b048e84003f8106c443f8718d
One hunk of this was reverted by Andrew in May:
https://gitlab.com/davical-project/awl/commit/4eb8f1bc91062296e0e24e8ef46a6403c70c2936
And I'm about to revert some more lines of it, in order to resurrect backslash
escaping
as well as escaping of semicolons for "other" properties (which fixes #837154):
--- a/inc/vProperty.php
+++ b/inc/vProperty.php
@@ -398,13 +398,15 @@ class vProperty extends vObject {
/** Content escaping does not apply to these properties culled
from RFC6350 / RFC2426 */
case 'ADR': case 'N': case 'ORG':
// escaping for ';' for these fields also needs to happen to the
components they are built from.
+ $escaped = str_replace( '\\', '\\\\', $escaped);
$escaped = preg_replace( '/\r?\n/', '\\n', $escaped);
$escaped = str_replace( ',', '\\,', $escaped);
break;
/** Content escaping applies by default to other properties */
default:
+ $escaped = str_replace( '\\', '\\\\', $escaped);
$escaped = preg_replace( '/\r?\n/', '\\n', $escaped);
- $escaped = preg_replace( "/([,])/", '\\\\$1', $escaped);
+ $escaped = preg_replace( "/([,;])/", '\\\\$1', $escaped);
}
.
$rendered = '';
I wonder if these parts of your commit were in fact not necessary to
achieve what you wanted to achieve, or if this will break your fix? Do
you have a test case, like an actual vCard or two, where you can explain
how it's supposed to be handled and what went wrong before your commit?
(I think it would be good to have more regression tests for vCard
handling, and to have the carddav suite fully passing, because this
seems to be a complicated area and from my cursory reading of the vCard
spec I think we may be cutting some corners currently. So it would be
good to have clearer documentation in the code what assumptions we make,
which parts of the spec we skip, how those lists of properties are
compiled from the mentioned RFCs etc., as well as the ability to rely on
regression testing to be sure we're not accidentally breaking stuff with
our fixes...)
Florian
