vcard
vcard copied to clipboard
jeroendesloovere
vCard 4.0: fix Name Property
fix Name Property (wrong order: FN and N have different orders!) mandatory check for fullname (fix #113 ) based on #128
@danger89 maybe you can help me
- [x] to pass travis checks
- [x] review the mandatory check
- [x] review the mandatory exception
- [x] confim that FN / N need that fix
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 :confused:
@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.
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?
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 :confused: (lack of knowledge of Github <> Travis integration I admit): https://github.com/helveticadomes/vcard/commit/4220a451e4b91de24b49795e4103f67f371a75ad
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.
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
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 :raised_hands: . 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.
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 :+1: .
@helveticadomes Do you like to join the Gitter chat? https://gitter.im/vcard-php-library/community This way we can easier help each other.
Ps.
@helveticadomes I don't get the "FullName is mandatory" exception when I use the getParameters(). But only with getContent().
@helveticadomes I don't get the "FullName is mandatory" exception when I use the
getParameters(). But only withgetContent().
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.
@helveticadomes I don't get the "FullName is mandatory" exception when I use the
getParameters(). But only withgetContent().if you use
getParameters()instead ofgetProperties()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
Currently its tested indirectly with several testcase like: https://github.com/jeroendesloovere/vcard/blob/9985584fdadd15761679e6e312719db0cf099d44/tests/VCardTest.php#L353
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).
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 :sob: , despite the standard followed):
Please, merge this PR, be more agile! :cake: Separate issue is created: https://github.com/jeroendesloovere/vcard/issues/148. Otherwise the merges/PRs are getting too big in my opinion.