Allow unknown value data types for VALUE #674
Conversation
Currently value-types that are not known will cause the parser to throw an InvalidDataException. In RFC5545, Section 3.2.20 is described though that the allowed values for the "VALUE" data-type can include x-names and iana-tokens. These can be any string that might - or might not - be understood by the parser. The description clearly states that "Applications MUST preserve the value data for x-name and iana-token value that they don't recognize without attempting to interpret or parse the value data" This means that the content of the "VALUE" part should not be interpreted at all the moment the parser doesn't find a match in the corresponding mapping table. But it should also not hard-fail when someone sets a VALUE that might not be understood. In such a case the default should be used instead.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #674 +/- ##
============================================
+ Coverage 98.76% 98.78% +0.02%
- Complexity 1874 1875 +1
============================================
Files 71 71
Lines 5343 5271 -72
============================================
- Hits 5277 5207 -70
+ Misses 66 64 -2 ☔ View full report in Codecov by Sentry. |
|
Background is a feed we got from a server that uses a The whole feed looks like this: Verifying this with the ICS-Validator at https://icalendar.org/validator.html results in "Success! No errors found"
Yet with the current implementation of the library importing this feed will fail due to the unknown |
THis adds a test to assure that an unknown value is skipped. The value `DATETIME` is not known. It should be `DATE-TIME`. Adding the value of DATETIME previopusly resulted in an Error. This now tests that it actually works and that the unknown parameter is actually also kept as specified by the RFC
This will remove the necesity to calculate the class name twice in immediate succession should the declared valuetype not be set.
heiglandreas
force-pushed
the
allow-unknown-value-data-types
branch
from
db22d0b
to
1f22f4a
Compare
| if (isset($parameters['VALUE'])) { | ||
| $class = $this->getClassNameForPropertyValue($parameters['VALUE']); | ||
| if (is_null($class)) { | ||
| throw new InvalidDataException('Unsupported VALUE parameter for '.$name.' property. You supplied "'.$parameters['VALUE'].'"'); |
There was a problem hiding this comment.
The code used to be able to throw this exception if $parameters['VALUE'] had an invalid value (did not end up matching to a class).
Now, if $parameters['VALUE'] is not valid, the code will end up doing:
$class = $this->getClassNameForPropertyName($name);
That is, it will just ignore the invalid value, and use what has been passed in $name.
There was a problem hiding this comment.
I am surprised that no unit test has failed. I guess that there is not a unit test that covers the (now deleted) InvalidDataException
There was a problem hiding this comment.
Yes. As far as I see this part of RFC5545, Section 3.2.20 (Value Data Type)
Applications MUST preserve the value data for x-name and iana-token values that they don't recognize without attempting to interpret or parse the value data.
we need to allow to keep - but not interpret - VALUE information that we don't understand. That didn't work previously due to the Exception. That broke the whole process of interpretation.
The Exception has been introduced with c20acd7 about 8 years ago to have "A better error message for unsupported VALUE parameters." but even then there should have been no Exception as unsupported VALUE parameters IMO do not pose an error condition. They should not be interpreted, yes (what they are now), but not break the process but also be preserved for other tools to interpret that might be able to understand it.
That is the reason why the current process then falls back to the default value when "an invalid" - let's rephrase that to "a not recognized" - value is encountered.
|
Backport to 4.5 branch is PR #699 |
Currently value-types that are not known will cause the parser to throw an InvalidDataException. In RFC5545, Section 3.2.20 is described though that the allowed values for the "VALUE" data-type can include x-names and iana-tokens. These can be any string that might - or might not - be understood by the parser. The description clearly states that "Applications MUST preserve the value data for x-name and iana-token value that they don't recognize without attempting to interpret or parse the value data"
This means that the content of the "VALUE" part should not be interpreted at all the moment the parser doesn't find a match in the corresponding mapping table.
But it should also not hard-fail when someone sets a VALUE that might not be understood. In such a case the default should be used instead.