-
Notifications
You must be signed in to change notification settings - Fork 193
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
vCard 4.0: fix Name Property #146
base: 2.0.0-dev
Are you sure you want to change the base?
Changes from 9 commits
0404f6f
17d55da
da3372a
690a942
1884522
543756d
390b6c8
52659dc
946f95a
aa1eddd
51ecc1b
9985584
061f052
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this also need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Z could be added indeed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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:
So I think the And the ISO 8601 is even revised. And have two different versions TLDR;To conclude: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, ..) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
} | ||
|
||
return $this->value; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ public function __construct(\DateTime $value) | |
|
||
public function __toString(): string | ||
{ | ||
// TODO: possibly not in line with rfc6350 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Thank you danger89 👍 Plus added an extra UT. |
||
return $this->value->format('u'); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace JeroenDesloovere\VCard; | ||
|
@@ -160,9 +159,38 @@ public function getParameters(string $filterByPropertyParameterClass = null): ar | |
}); | ||
} | ||
|
||
/** | ||
* @param string $forPropertyClass | ||
* @return array | ||
* @throws VCardException | ||
*/ | ||
public function getProperties(string $forPropertyClass = null): array | ||
{ | ||
if ($forPropertyClass === null) { | ||
$array = $this->properties; | ||
//make empty array var for each required Property or Property you need to work with | ||
$found_fullname = $found_name = $others = []; | ||
foreach ($array as $value) { | ||
//search if property exist and add it to the defined array var, else add it to the others array | ||
if ($value instanceof FullName) { | ||
$found_fullname[] = $value; | ||
} elseif ($value instanceof Name) { | ||
$found_name[] = $value; | ||
} else { | ||
$others[] = $value; | ||
} | ||
} | ||
//check for empty and throw exception, if it can not be generated by existing fields | ||
if (count($found_fullname) == 0) { | ||
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())))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(),
)))); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will fix this for helveticadomes. |
||
} | ||
} | ||
$array = array_merge($found_fullname, $found_name, $others); | ||
$this->properties = $array; | ||
|
||
return $this->properties; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use spaces not tabs just like the rest of the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ow oopsy thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your welcome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lessons learned: never use Gedit, Atom & vim all together 🤣