vCard 4.0: fix Name Property #146

helveticadomes · 2019-03-04T10:09:21Z

fix Name Property (wrong order: FN and N have different orders!)
mandatory check for fullname (fix #113 ) based on #128

…atory check for fullname

helveticadomes · 2019-03-04T10:31:12Z

@Danger89 maybe you can help me

to pass travis checks
review the mandatory check
review the mandatory exception
confim that FN / N need that fix

melroy89 · 2019-03-04T17:05:55Z

Ah the name parser was also wrong. So the order was indeed wrong regarding lastname, firstname and additional.

It's also a best practice to add some additional unit testers with introducing new code.
Indeed the RFC 6350 says regarding FN: "The property MUST be present in the vCard object."

While the N (Name) SHOULD be present.

Is is OK to assume that when N is filled-in, FN can be derived from that? So still ending up in the vCard, without explicitly setting this? Since, this will be my use case. With the option to set the FN manually whenever the user likes to set FN differently.

Eg. When N:Doe;J.;;; is specified FN is generated for you, even if FN is not specified, to the vCard output (.vcf): FN:J. Doe.

However, when both FN & N is not filled-in, only then return an error that FN is mandatory. Or is that already implemented like such 😕

melroy89 · 2019-03-04T22:09:41Z

@helveticadomes I changed my unit tester on the dev branch, can you pull the new changes from 2.0.0-dev? So I can validate the FN outcome.

helveticadomes · 2019-03-05T13:06:40Z

Is it OK to assume that when N is filled-in, FN can be derived from that?

for me that make most sense, or do you think i should not do that?

it is implemented that:
if FN exist and N exist => keep both and do nothing to them
if FN exist but N dont exist => keep FN and do nothing with N
if FN & N dont exist => throw exception that FN is mandatory
if FN dont exist but N exist => silent create FN from N, and add it to the vcard

pull which changes where and how?

melroy89 · 2019-03-05T13:23:09Z

it is implemented that:
if FN exist and N exist => keep both and do nothing to them
if FN exist but N dont exist => keep FN and do nothing with N
if FN & N dont exist => throw exception that FN is mandatory
if FN dont exist but N exist => silent create FN from N, and add it to the vcard

That sounds good indeed!

pull which changes where and how?

You already did :). But how to trigger a rebuild on this PR 😕 (lack of knowledge of Github <> Travis integration I admit):
helveticadomes@4220a45

…atory check for fullname

…es/vcard into feature-fixed-name-property

melroy89 · 2019-03-05T13:44:56Z

is it a good idea to also add a correct vcard file to assets? With correct I mean define the FN:.
https://github.com/helveticadomes/vcard/blob/feature-fixed-name-property/tests/assets/vcard.vcf

We can keep the current vcard asset as well (rename it?), since I think its a good test to validate if the jeroendesloovere/vcard parser is robust enough for parsing vcards files without FN (and only N) -> although its is an invalid Vcard file I think the parser should be robust.

melroy89 · 2019-03-05T13:56:53Z

src/VCard.php

+                if (count($found_name) == 0) {
+                    throw VCardException::forRequiredProperty(new FullName('NoName'));
+                } else {
+                    $found_fullname[] = new FullName(implode(' ', array_filter(array($found_name[0]->getPrefix(), $found_name[0]->getLastName(), $found_name[0]->getAdditional(), $found_name[0]->getFirstName(), $found_name[0]->getSuffix()))));


Looking at the result of the fullname of the unit tester ran https://travis-ci.org/jeroendesloovere/vcard/jobs/502040710.

+FN:Mr. van den Berg Antoine Melroy\r\n

This is not correct.

I prefer: Prefix - firstname - additional - lastname - suffix order.

Which should result into:
+FN:Mr. Melroy Antoine van den Berg\r\n

I hope you agree as well regarding this.

i agree, fixed that & forcepush was to remove the commits created form a wrong rebase

helveticadomes · 2019-03-05T15:44:05Z

lack of knowledge that describe exactly me :-D (git/github/travis)
if i assume right something need to be added to VCardTest.php because FN is added if only N exist.
but with all this dependencies it is something that begin slightly overstrain my capability

…es/vcard into feature-fixed-name-property

melroy89 · 2019-03-05T16:09:07Z

lack of knowledge that describe exactly me :-D (git/github/travis)
if i assume right something need to be added to VCardTest.php because FN is added if only N exist.
but with all this dependencies it is something that begin slightly overstrain my capability

No worries. I can either do it myself. Or better, guide you through the process to help you. So next time you do have the ability and skills to do it yourself 🙌 . I like the second approach, and help you.

EDIT: I think that unit testing is part of software development. So I think its a good think that can you write & execute them as well.

EDIT 2: I requested via @jeroendesloovere a gitter.im community project for vcard, so we could easily chat. Which may help in communication and learn faster.

…g in the BDAY parser (change production code to fix). The testcases is correct.. 1 error left to fix as well

… mind to validate DateTimeValue class as well!

melroy89 · 2019-03-06T22:17:55Z

I fixed the testcases, found yet another bug in the DateTimeOrStringValue class. regarding the __toString() method.

The 'u' format (see Date format in php docs) will return zero's with a date! So that is definitely not the behavior you want.

Therefore I urge you to also look to the DateTimeValue which properly also need the same fix as DateTimeOrStringValue?

I also created a composer test, composer standard and finally a composer fix-standard. You're welcome 👍 .

to-be-the-best · 2019-03-06T22:08:30Z

src/VCard.php

+                if (count($found_name) == 0) {
+                    throw VCardException::forRequiredProperty(new FullName('NoName'));
+                } else {
+                    $found_fullname[] = new FullName(implode(' ', array_filter(array($found_name[0]->getPrefix(), $found_name[0]->getFirstName(), $found_name[0]->getAdditional(), $found_name[0]->getLastName(), $found_name[0]->getSuffix()))));


Can this be made more readable, for example something like:

$found_fullname[] = new FullName(implode(' ', array_filter(array( $found_name[0]->getPrefix(), $found_name[0]->getFirstName(), $found_name[0]->getAdditional(), $found_name[0]->getLastName(), $found_name[0]->getSuffix(), ))));

I will fix this for helveticadomes.

to-be-the-best · 2019-03-06T22:27:17Z

composer.json

+    },
+    "scripts": {
+        "test": "phpunit tests",
+	"standard": "phpcs --standard=psr2 --extensions=php --warning-severity=0 --report=full 'src'",


Please use spaces not tabs just like the rest of the file

ow oopsy thx

Your welcome

Lessons learned: never use Gedit, Atom & vim all together 🤣

to-be-the-best · 2019-03-06T22:44:23Z

tests/VCardTest.php

-    private $secondVCard;
+    protected static $secondVCard;
+
+    /** @var VCards: containing both first & second vcard together */


should this not be of the type VCard?
https://github.com/jeroendesloovere/vcard/pull/146/files#diff-fc15453e7e481d3e92f86a7d1d2e7a99R101 here it's set with VCard

Also i'm not sure, but i don't believe the : is needed/allowed in the phpdoc specs

ya ok its type Vcard.. I wanted to point out this variable stores two vcards together.

I don't know what I was thinking.. I will revert it.

melroy89 · 2019-03-07T17:37:16Z

@jeroendesloovere I think it's ready to be merged.

melroy89 · 2019-03-07T17:39:26Z

src/Property/Value/DateTimeValue.php

@@ -16,6 +16,7 @@ public function __construct(\DateTime $value)

    public function __toString(): string
    {
+        // TODO: possibly not in line with rfc6350


However, somebody could also look to this toString() method. However, this can be done later as well (has nothing to do with the Name property fix)

Fixed. Thank you danger89 👍 Plus added an extra UT.

melroy89 · 2019-03-07T20:20:44Z

@helveticadomes Do you like to join the Gitter chat? https://gitter.im/vcard-php-library/community
This way we can easier help each other.

melroy89 · 2019-03-07T20:35:13Z

Ps.
@helveticadomes I don't get the "FullName is mandatory" exception when I use the getParameters(). But only with getContent().

to-be-the-best · 2019-03-07T21:57:06Z

src/Property/Value/DateTimeOrStringValue.php

@@ -17,7 +17,8 @@ public function __construct($value)
    public function __toString(): string
    {
        if ($this->value instanceof \DateTime) {
-            return $this->value->format('u');
+            // According to DATE-AND-OR-TIME rfc6350 standard
+            return $this->value->format('Ymd\THis');


Does this also need the z just like https://github.com/jeroendesloovere/vcard/pull/146/files#diff-0a46c638422a54cb3e31646555c0c263R20

Z could be added indeed.

to-be-the-best · 2019-03-07T22:24:59Z

src/Property/Value/DateTimeOrStringValue.php

@@ -17,7 +17,8 @@ public function __construct($value)
    public function __toString(): string
    {
        if ($this->value instanceof \DateTime) {
-            return $this->value->format('u');
+            // According to DATE-AND-OR-TIME rfc6350 standard
+            return $this->value->format('Ymd\THis');


I also want to raise a point for discussion, I see that in rfc6350 all the references to date and/or time basically say to use ISO 8601.
The thing is that php has a constant for ISO 8601 (DateTimeInterface::ATOM) which has the following syntax: Y-m-d\TH:i:sP (example: 2005-08-15T15:52:01+00:00), but this is different from the examples in rfc6350.
Personally i prefer the DateTimeInterface::ATOM style.
I'm interested in your opinions.

@to-be-the-best The RFC6350 says it's BASED ON that ISO standard: and that ISO standard mainly says this is the standard ISO notation: YYYY-MM-DD. Bear in mind that the ISO standard is very brought, and is not specific enough. That is why the RFC6350 is extended this ISO standard, to be more clear.

I'm just looking at the specification of that document:

To allow unambiguous
interpretation, a stand-alone TIME value is always preceded by a "T".

Examples for "date-and-or-time":

19961022T140000

So I think the Z is debatable indeed.

And the ISO 8601 is even revised. And have two different versions ISO/DIS 8601-1 & ISO/DIS 8601-2.

TLDR;To conclude: Ymd\THis is OK according to the RFC6350 spec. The RFC standard is based on the ISO standard, but extends that, therefor I like to look to RFC standard only. Since RFC6350 is leading.
Finally, I think '\Z' can be added if you wish.

Ps. plus we need to do manual test procedures to validate if this notation is indeed working for the most used applications (eg. Outlook, Google Contacts, iCloud, ..)

Ps. ps. Actually another related discussion I like to have instead, is how to define a PHP class that can be used to only output Ymd? Since the PHP DateTime() can be used to store both date & time. So I can only assume now that there WILL be a time included (also 00:00:00 could even be valid) and dump the full date time RFC standard notation into my vcard.

However, it would be nice to have an option to only a date (without time) and time (without date), with the proper toString() implementation for those two cases (together with the full date time case-> already implemented).

Use cases are: Birthday or anniversary, where just a date, without time is sufficient enough in most cases.

helveticadomes · 2019-03-08T07:26:46Z

@helveticadomes I don't get the "FullName is mandatory" exception when I use the getParameters(). But only with getContent().

if you use getParameters() instead of getProperties() it is clear that you would not get the FullName exception ;-)

PS: i assume we need to add this expection also to VCardTest.

to-be-the-best · 2019-03-08T11:13:07Z

@helveticadomes I don't get the "FullName is mandatory" exception when I use the getParameters(). But only with getContent().

if you use getParameters() instead of getProperties() it is clear that you would not get the FullName exception ;-)

PS: i assume we need to add this expection also to VCardTest.

Indeed the expection needs tests

melroy89 · 2019-03-08T16:26:39Z

Currently its tested indirectly with several testcase like:

vcard/tests/VCardTest.php

Line 353 in 9985584

public function testVersionParameterParserGoodWeather(): void

As you can see these tests are not resulting into an exception/error. But indeed you want to have a test that proves the exception is actually trigger also (bad weather test).

melroy89 · 2019-03-08T16:34:32Z

Ready to merge now..? Please say yes ❤️

melroy89 · 2019-03-08T17:15:57Z

Create issue regarding DateTime to support also a single date or time object (with a proper toString() implementation for these two cases). Currently the DateTime can store both a date and time (even 00:00:00 is a valid time), therefor we always output the full date + time to the vcard file currently.

However Google Contacts can't handle this actually, despite the RFC standard. Regarding the birthday:
BDAY:19891207 results into (GOOD):

BDAY:19891207T104310 Results into (Not good 😭 , despite the standard followed):

Please, merge this PR, be more agile! 🍰 Separate issue is created: #148. Otherwise the merges/PRs are getting too big in my opinion.

fix Name Property (wrong order: FN and N have different orders!) mand…

0404f6f

…atory check for fullname

helveticadomes added 2 commits March 5, 2019 14:26

fix Name Property (wrong order: FN and N have different orders!) mand…

17d55da

…atory check for fullname

Merge branch 'feature-fixed-name-property' of github.com:helveticadom…

da3372a

…es/vcard into feature-fixed-name-property

melroy89 suggested changes Mar 5, 2019

View reviewed changes

helveticadomes force-pushed the feature-fixed-name-property branch from da3372a to 17d55da Compare March 5, 2019 15:37

change firstname with lastname, like requested

690a942

Merge branch 'feature-fixed-name-property' of github.com:helveticadom…

1884522

…es/vcard into feature-fixed-name-property

Melroy van den Berg and others added 3 commits March 5, 2019 20:50

Add extra testcases. Defect in vcf files, changed as well. Still a bu…

543756d

…g in the BDAY parser (change production code to fix). The testcases is correct.. 1 error left to fix as well

vcard 1 & 2 added (I forgot to add them)

390b6c8

DateTimeOrStringValue format fixed, now according to rfc6350. bear in…

52659dc

… mind to validate DateTimeValue class as well!

Travis is hanging. trigger another build??

946f95a

to-be-the-best reviewed Mar 6, 2019

View reviewed changes

Code layout fixes

aa1eddd

to-be-the-best reviewed Mar 6, 2019

View reviewed changes

Improve UT

51ecc1b

melroy89 approved these changes Mar 7, 2019

View reviewed changes

melroy89 reviewed Mar 7, 2019

View reviewed changes

Fix datatime value return value of to_string.

9985584

to-be-the-best reviewed Mar 7, 2019

View reviewed changes

Test FullName mandatory exception test

061f052

melroy89 mentioned this pull request Mar 8, 2019

Support Time and Date solely next to DateTime (2.0.0-dev) #148

Open

helveticadomes mentioned this pull request Mar 21, 2019

Create method to check "This is the only mandatory property." #113

Open

helveticadomes requested a review from jeroendesloovere February 19, 2020 13:51

vCard 4.0: fix Name Property #146

Are you sure you want to change the base?

vCard 4.0: fix Name Property #146

Conversation

helveticadomes commented Mar 4, 2019

helveticadomes commented Mar 4, 2019 • edited Loading

melroy89 commented Mar 4, 2019 • edited Loading

melroy89 commented Mar 4, 2019 • edited Loading

helveticadomes commented Mar 5, 2019

melroy89 commented Mar 5, 2019 • edited Loading

melroy89 commented Mar 5, 2019

Choose a reason for hiding this comment

helveticadomes Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

helveticadomes commented Mar 5, 2019

melroy89 commented Mar 5, 2019 • edited Loading

melroy89 commented Mar 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melroy89 commented Mar 7, 2019

Choose a reason for hiding this comment

melroy89 Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

melroy89 commented Mar 7, 2019

melroy89 commented Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

melroy89 Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melroy89 Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melroy89 Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

helveticadomes commented Mar 8, 2019 • edited Loading

to-be-the-best commented Mar 8, 2019

melroy89 commented Mar 8, 2019

melroy89 commented Mar 8, 2019

melroy89 commented Mar 8, 2019 • edited Loading

helveticadomes commented Mar 4, 2019 •

edited

Loading

melroy89 commented Mar 4, 2019 •

edited

Loading

melroy89 commented Mar 4, 2019 •

edited

Loading

melroy89 commented Mar 5, 2019 •

edited

Loading

helveticadomes Mar 5, 2019 •

edited

Loading

melroy89 commented Mar 5, 2019 •

edited

Loading

melroy89 Mar 7, 2019 •

edited

Loading

melroy89 commented Mar 7, 2019 •

edited

Loading

melroy89 Mar 8, 2019 •

edited

Loading

melroy89 Mar 8, 2019 •

edited

Loading

melroy89 Mar 8, 2019 •

edited

Loading

helveticadomes commented Mar 8, 2019 •

edited

Loading

melroy89 commented Mar 8, 2019 •

edited

Loading