Y2038 Problems? #1826
Comments
5 tasks
MarkBaker
pushed a commit
that referenced
this issue
Feb 14, 2021
This avoids a potential Y2038 problem on 32-bit systems - see issue #1826.
|
PR #1891 addresses two specific problems. |
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this issue
May 23, 2021
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.
5 tasks
MarkBaker
pushed a commit
that referenced
this issue
May 30, 2021
* Document Properties - Coverage and 32-bit-safe Timestamps 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 #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 #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. * Phpstan 6 baseline deletions, 2 docblock changes * Scrutinizer's Turn 3 minor errors that hadn't blocked the request.
|
PR #2113 makes "date created", "date modified", and custom date properties 32-bit safe. The following uses of strtime remain in src:
|
|
Use of strtotime in Reader/Ods is 32-bit safe, so only the other 3 need to be addressed. |
|
Gnumeric and XML PR's are ready. Autofilter will take a lot more time. |
MarkBaker
pushed a commit
that referenced
this issue
May 31, 2021
* Document Properties - Coverage and 32-bit-safe Timestamps 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 #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 #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. * Phpstan 6 baseline deletions, 2 docblock changes * Scrutinizer's Turn 3 minor errors that hadn't blocked the request. * Gnumeric Reader - Distinguish Created and Modified Timestamps Both are being used to set both fields; change to set the appropriate one in each case. Also replace use of 32-bit-unsafe strtotime.
MarkBaker
pushed a commit
that referenced
this issue
May 31, 2021
* Document Properties - Coverage and 32-bit-safe Timestamps 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 #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 #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. * Phpstan 6 baseline deletions, 2 docblock changes * Scrutinizer's Turn 3 minor errors that hadn't blocked the request. * Reader XML Properties - Eliminate strtotime Piggyback on top of prior changes to eliminate 32-bit-unsafe call. Add explicit tests for created, modified, and custom date properties.
|
Gnumeric and XML now taken care of. Working on AutoFilter. |
|
Filed bug report https://bugs.php.net/bug.php?id=81287 for PHP8.1 implementation of DateTimeZone::getTransitions. Its use is already replaced with an equivalent working function in PhpSpreadsheet. |
|
Potential problems are fixed. I could certainly be overlooking something, but I think this can be closed now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is:
What is the expected behavior?
Software should continue to work after epoch change in 2038.
What is the current behavior?
Running on 64-bit Php is probably okay. 32-bit may have problems.
What are the steps to reproduce?
Too early? I think not. YMMV. The analysis below is probably not complete, but it's a start.
PHP itself has some Y2038 exposure (e.g. see https://bugs.php.net/bug.php?id=64992). Dealing with these is clearly out of scope for PhpSpreadsheet.
I think that PhpSpreadsheet is pretty well positioned to avoid problems. However, it does use functions mktime, gmmktime, and strtotime. Those functions will work just fine after the Y2038 transition when 64-bit PHP is used. However, they will fail when 32-bit PHP is used. It is hard to guess if 32-bit PHP will even be a thing when the time comes, but it's best to be prepared. I don't know that it is possible to make 64-bit a requirement, and, even if it is, doing that seems far too drastic. On the other hand, it should be possible to have the unit tests run in a 32-bit region. I don't know how to arrange that. I would suggest that it is probably sufficient to run one 32-bit region rather than a 32-bit version of each supported release of PHP.
Scanning the source code, I find the following use of mktime/gmmktime:
The use of strtotime is more extensive. I haven't researched any of these:
Scanning the vendor directory, the only uses of mktime/gmmktime that I see are in jpgraph, which is already problematic because it isn't supported through Composer. Again, I think this is out of scope. There is more extensive use of strtotime:
My reading of the documentation for the strftime, localtime, date, and time functions, which take a timestamp argument, suggests that they may also be problematic after the epoch, especially when the timestamp argument is omitted. If so, this is probably a problem that PHP needs to address. For the record, Calculation/DateTime uses some of these.
Which versions of PhpSpreadsheet and PHP are affected?
AFAIK, only 32-bit PHP is affected.
The text was updated successfully, but these errors were encountered: