Skip to content

Commit

Permalink
Document Properties - Coverage and 32-bit-safe Timestamps
Browse files Browse the repository at this point in the history
While researching an issue, I noticed that coverage of Document/Properties was poor. Further, the use of int timestamps will eventually lead to problems for 32-bit PHP (see issue PHPOffice#1826).

Coverage Changes:
- Many property types with no special handling are enumerated but not tested. These are removed, but will continue to function as before.
- Existing code theoretically allows property to be set to an object, but there is no means to read or write such a property, and, even if there were, I don't believe Excel supports it. Setting a property to an object will now be changed to a no-op (can throw an exception if preferred).
- Since the Properties object now has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed.
- Large switch statements are replaced with associative arrays. Scrutinizer will like that.
- Coverage is now 100%.

<!-- end of coverage changes list -->

Timestamp Changes:
- Timestamps will be stored as int if possible, or float if not. This is, or will soon be, needed for 32-bit systems. Tests have been added for beyond-epoch dates, and run successfully with 32-bit.
- LibreOffice doesn't quite get the Created/Modified properties correct. These are written to the file as a string which includes offset from UTC, but LibreOffice ignores the offset portion when displaying them. Code had been generating these in UTC, but now generates them in default timezone, which should meet user's expectations.

<!-- end of timestamp changes list -->

Other Changes:
- Custom properties added to ODS Writer.
- Samples had not been generating any ODS files. One is now generated.
- Ods uses a single 'keywords' property rather than multiple 'keyword' properties.
- Breaking change - default company is changed to null string from Microsoft Corporation.
- Breaking change of sorts - PropertiesTest incorrectly tested a custom date property against a string, Reader/XlsxTest correctly tested against a timestamp converted to a string. PropertiesTest was defective, and will no longer work as coded; anyone using it as a model will likewise have a problem.
- PHP8.1 has been complaining for weeks about a time zone conversion test. I have now downloaded a version, and changed the code so that it will work in 8.1 as well as prior releases. (It is still likely that the existing code should work in 8.1, but I haven't yet figured out how to file a bug report.) In the course of testing, 3 additional 8.1 problems were reported (all along the lines of "can't pass null to strpos"), and are fixed with null coercion.
- Two Calculation tests failed because of large results on 32-bit system. These are corrected by allowing the functions involved to return float|int rather than int. I suspect that there are other functions with this problem, and will investigate as a follow-up activity.
- See issue PHPOffice#2090. I believe that changes between 17.1 and master will merely cause the problematic spreadsheet to fail in a different way. I believe that enclosing in quotes some variables passed to Document/Properties by Reader/Xlsx will eliminate the problem, but, in the absence of an example file, cannot say for sure.
- Properties tests are now separated out from Reader/XlsxTest and Reader/OdsTest, and now test both Read and Write (via reload).

<!-- end of other changes list -->

Miscellaneous Notes:
- There remains no support for Custom Properties in Xls Reader or Writer.
- We now have default timezones for all of PHP itself, Shared/Date, and Shared/Timezone. That is least one too many. I was unable to disentangle the latter two for this change, but will look into deprecating one or the other in future.
  • Loading branch information
oleibman committed May 23, 2021
1 parent b05dc31 commit cad1730
Show file tree
Hide file tree
Showing 28 changed files with 634 additions and 472 deletions.
145 changes: 0 additions & 145 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -1270,11 +1270,6 @@ parameters:
count: 1
path: src/PhpSpreadsheet/Calculation/MathTrig/Arabic.php

-
message: "#^Parameter \\#1 \\$number of function base_convert expects string, int\\<0, 9007199254740991\\> given\\.$#"
count: 1
path: src/PhpSpreadsheet/Calculation/MathTrig/Base.php

-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\MathTrig\\\\Helpers\\:\\:validateNumericNullBool\\(\\) should return float\\|int but returns float\\|int\\|string\\.$#"
count: 1
Expand Down Expand Up @@ -2490,36 +2485,6 @@ parameters:
count: 1
path: src/PhpSpreadsheet/DefinedName.php

-
message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:\\$created \\(int\\) does not accept int\\|false\\.$#"
count: 1
path: src/PhpSpreadsheet/Document/Properties.php

-
message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:\\$modified \\(int\\) does not accept int\\|false\\.$#"
count: 1
path: src/PhpSpreadsheet/Document/Properties.php

-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:identifyPropertyType\\(\\) has no return typehint specified\\.$#"
count: 1
path: src/PhpSpreadsheet/Document/Properties.php

-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:identifyPropertyType\\(\\) has parameter \\$propertyValue with no typehint specified\\.$#"
count: 1
path: src/PhpSpreadsheet/Document/Properties.php

-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:convertProperty\\(\\) has no return typehint specified\\.$#"
count: 1
path: src/PhpSpreadsheet/Document/Properties.php

-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:convertProperty\\(\\) has parameter \\$propertyValue with no typehint specified\\.$#"
count: 1
path: src/PhpSpreadsheet/Document/Properties.php

-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\DocumentGenerator\\:\\:getPhpSpreadsheetFunctionText\\(\\) has parameter \\$functionCall with no typehint specified\\.$#"
count: 1
Expand Down Expand Up @@ -3180,16 +3145,6 @@ parameters:
count: 1
path: src/PhpSpreadsheet/Reader/Ods/Properties.php

-
message: "#^Parameter \\#1 \\$timestamp of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setCreated\\(\\) expects int\\|string\\|null, int\\|false given\\.$#"
count: 2
path: src/PhpSpreadsheet/Reader/Ods/Properties.php

-
message: "#^Parameter \\#1 \\$timestamp of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setModified\\(\\) expects int\\|string\\|null, int\\|false given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Ods/Properties.php

-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Ods\\\\Properties\\:\\:setMetaProperties\\(\\) has parameter \\$namespacesMeta with no typehint specified\\.$#"
count: 1
Expand Down Expand Up @@ -3405,61 +3360,6 @@ parameters:
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$codePage of static method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\CodePage\\:\\:numberToName\\(\\) expects int, int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$title of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setTitle\\(\\) expects string, int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$subject of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setSubject\\(\\) expects string, int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$creator of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setCreator\\(\\) expects string, int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$keywords of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setKeywords\\(\\) expects string, int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$description of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setDescription\\(\\) expects string, int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$modifier of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setLastModifiedBy\\(\\) expects string, int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$codePage of static method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\CodePage\\:\\:numberToName\\(\\) expects int, bool\\|int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$category of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setCategory\\(\\) expects string, bool\\|int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$manager of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setManager\\(\\) expects string, bool\\|int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#1 \\$company of method PhpOffice\\\\PhpSpreadsheet\\\\Document\\\\Properties\\:\\:setCompany\\(\\) expects string, bool\\|int\\|string\\|null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Reader/Xls.php

-
message: "#^Parameter \\#3 \\$subject of function str_replace expects array\\|string, int\\|string\\|null given\\.$#"
count: 1
Expand Down Expand Up @@ -5250,16 +5150,6 @@ parameters:
count: 1
path: src/PhpSpreadsheet/Shared/OLE.php

-
message: "#^Parameter \\#7 \\$time_1st of class PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\OLE\\\\PPS constructor expects int, null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Shared/OLE.php

-
message: "#^Parameter \\#8 \\$time_2nd of class PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\OLE\\\\PPS constructor expects int, null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Shared/OLE.php

-
message: "#^Parameter \\#9 \\$data of class PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\OLE\\\\PPS constructor expects string, null given\\.$#"
count: 1
Expand Down Expand Up @@ -5330,16 +5220,6 @@ parameters:
count: 1
path: src/PhpSpreadsheet/Shared/OLE/PPS/File.php

-
message: "#^Parameter \\#7 \\$time_1st of method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\OLE\\\\PPS\\:\\:__construct\\(\\) expects int, null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Shared/OLE/PPS/File.php

-
message: "#^Parameter \\#8 \\$time_2nd of method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\OLE\\\\PPS\\:\\:__construct\\(\\) expects int, null given\\.$#"
count: 1
path: src/PhpSpreadsheet/Shared/OLE/PPS/File.php

-
message: "#^Parameter \\#1 \\$No of method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\OLE\\\\PPS\\:\\:__construct\\(\\) expects int, null given\\.$#"
count: 1
Expand Down Expand Up @@ -5485,11 +5365,6 @@ parameters:
count: 1
path: src/PhpSpreadsheet/Shared/StringHelper.php

-
message: "#^Else branch is unreachable because previous condition is always true\\.$#"
count: 1
path: src/PhpSpreadsheet/Shared/TimeZone.php

-
message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Trend\\\\BestFit\\:\\:\\$goodnessOfFit has no typehint specified\\.$#"
count: 1
Expand Down Expand Up @@ -8470,31 +8345,11 @@ parameters:
count: 1
path: tests/PhpSpreadsheetTests/Reader/Xml/XmlTest.php

-
message: "#^Parameter \\#1 \\$timeZone of static method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Date\\:\\:setDefaultTimezone\\(\\) expects DateTimeZone\\|string, DateTimeZone\\|null given\\.$#"
count: 1
path: tests/PhpSpreadsheetTests/Shared/DateTest.php

-
message: "#^Parameter \\#1 \\$pValue of static method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\StringHelper\\:\\:setCurrencyCode\\(\\) expects string, null given\\.$#"
count: 1
path: tests/PhpSpreadsheetTests/Shared/StringHelperTest.php

-
message: "#^Parameter \\#1 \\$timeZone of static method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Date\\:\\:setDefaultTimezone\\(\\) expects DateTimeZone\\|string, DateTimeZone\\|null given\\.$#"
count: 1
path: tests/PhpSpreadsheetTests/Shared/TimeZoneTest.php

-
message: "#^Cannot call method getTimestamp\\(\\) on DateTime\\|false\\.$#"
count: 2
path: tests/PhpSpreadsheetTests/Shared/TimeZoneTest.php

-
message: "#^Parameter \\#1 \\$timezone of static method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\TimeZone\\:\\:getTimeZoneAdjustment\\(\\) expects string, null given\\.$#"
count: 1
path: tests/PhpSpreadsheetTests/Shared/TimeZoneTest.php

-
message: "#^Method PhpOffice\\\\PhpSpreadsheetTests\\\\SpreadsheetTest\\:\\:testGetSheetByName\\(\\) has parameter \\$index with no typehint specified\\.$#"
count: 1
Expand Down
2 changes: 1 addition & 1 deletion samples/Basic/01_Simple.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,4 @@
->setTitle('Simple');

// Save
$helper->write($spreadsheet, __FILE__);
$helper->write($spreadsheet, __FILE__, ['Xlsx', 'Xls', 'Ods']);
6 changes: 3 additions & 3 deletions src/PhpSpreadsheet/Calculation/Calculation.php
Original file line number Diff line number Diff line change
Expand Up @@ -4562,7 +4562,7 @@ private function processTokenStack($tokens, $cellID = null, ?Cell $pCell = null)
} else {
$this->executeNumericBinaryOperation($multiplier, $arg, '*', 'arrayTimesEquals', $stack);
}
} elseif (preg_match('/^' . self::CALCULATION_REGEXP_CELLREF . '$/i', $token, $matches)) {
} elseif (preg_match('/^' . self::CALCULATION_REGEXP_CELLREF . '$/i', $token ?? '', $matches)) {
$cellRef = null;
if (isset($matches[8])) {
if ($pCell === null) {
Expand Down Expand Up @@ -4637,7 +4637,7 @@ private function processTokenStack($tokens, $cellID = null, ?Cell $pCell = null)
}

// if the token is a function, pop arguments off the stack, hand them to the function, and push the result back on
} elseif (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/miu', $token, $matches)) {
} elseif (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/miu', $token ?? '', $matches)) {
if ($pCellParent) {
$pCell->attach($pCellParent);
}
Expand Down Expand Up @@ -4727,7 +4727,7 @@ private function processTokenStack($tokens, $cellID = null, ?Cell $pCell = null)
}
} else {
// if the token is a number, boolean, string or an Excel error, push it onto the stack
if (isset(self::$excelConstants[strtoupper($token)])) {
if (isset(self::$excelConstants[strtoupper($token ?? '')])) {
$excelConstant = strtoupper($token);
$stack->push('Constant Value', self::$excelConstants[$excelConstant]);
if (isset($storeKey)) {
Expand Down
4 changes: 2 additions & 2 deletions src/PhpSpreadsheet/Calculation/MathTrig/Base.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class Base
public static function evaluate($number, $radix, $minLength = null)
{
try {
$number = (int) Helpers::validateNumericNullBool($number);
$number = (float) floor(Helpers::validateNumericNullBool($number));
$radix = (int) Helpers::validateNumericNullBool($radix);
} catch (Exception $e) {
return $e->getMessage();
Expand All @@ -36,7 +36,7 @@ public static function evaluate($number, $radix, $minLength = null)
return Functions::NAN(); // Numeric range constraints
}

$outcome = strtoupper((string) base_convert($number, 10, $radix));
$outcome = strtoupper((string) base_convert("$number", 10, $radix));
if ($minLength !== null) {
$outcome = str_pad($outcome, (int) $minLength, '0', STR_PAD_LEFT); // String padding
}
Expand Down
2 changes: 1 addition & 1 deletion src/PhpSpreadsheet/Calculation/Statistical.php
Original file line number Diff line number Diff line change
Expand Up @@ -1402,7 +1402,7 @@ public static function PERCENTRANK($valueSet, $value, $significance = 3)
* @param int $numObjs Number of different objects
* @param int $numInSet Number of objects in each permutation
*
* @return int|string Number of permutations, or a string containing an error
* @return float|int|string Number of permutations, or a string containing an error
*/
public static function PERMUT($numObjs, $numInSet)
{
Expand Down
13 changes: 9 additions & 4 deletions src/PhpSpreadsheet/Calculation/Statistical/Permutations.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PhpOffice\PhpSpreadsheet\Calculation\Exception;
use PhpOffice\PhpSpreadsheet\Calculation\Functions;
use PhpOffice\PhpSpreadsheet\Calculation\MathTrig;
use PhpOffice\PhpSpreadsheet\Shared\IntOrFloat;

class Permutations
{
Expand All @@ -20,7 +21,7 @@ class Permutations
* @param mixed $numObjs Integer number of different objects
* @param mixed $numInSet Integer number of objects in each permutation
*
* @return int|string Number of permutations, or a string containing an error
* @return float|int|string Number of permutations, or a string containing an error
*/
public static function PERMUT($numObjs, $numInSet)
{
Expand All @@ -38,7 +39,9 @@ public static function PERMUT($numObjs, $numInSet)
return Functions::NAN();
}

return (int) round(MathTrig\Fact::evaluate($numObjs) / MathTrig\Fact::evaluate($numObjs - $numInSet));
$result = round(MathTrig\Fact::evaluate($numObjs) / MathTrig\Fact::evaluate($numObjs - $numInSet));

return IntOrFloat::evaluate($result);
}

/**
Expand All @@ -50,7 +53,7 @@ public static function PERMUT($numObjs, $numInSet)
* @param mixed $numObjs Integer number of different objects
* @param mixed $numInSet Integer number of objects in each permutation
*
* @return int|string Number of permutations, or a string containing an error
* @return float|int|string Number of permutations, or a string containing an error
*/
public static function PERMUTATIONA($numObjs, $numInSet)
{
Expand All @@ -68,6 +71,8 @@ public static function PERMUTATIONA($numObjs, $numInSet)
return Functions::NAN();
}

return (int) ($numObjs ** $numInSet);
$result = $numObjs ** $numInSet;

return IntOrFloat::evaluate($result);
}
}
Loading

0 comments on commit cad1730

Please sign in to comment.