Skip to content
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

Remove obsolete Date/Time library #970

Closed
dregad opened this issue May 18, 2023 · 18 comments · Fixed by #1002
Closed

Remove obsolete Date/Time library #970

dregad opened this issue May 18, 2023 · 18 comments · Fixed by #1002
Assignees
Labels
code cleanup datetime ADOdb Date/Time Library
Milestone

Comments

@dregad
Copy link
Member

dregad commented May 18, 2023

It does not make much sense anymore in the age of 64-bit computing, where timestamps will not overflow for 292 billion years, and it relies on use of strftime()/gmstrftime(), which have been deprecated in PHP 8.1.

Follows discussion in #963

@dregad dregad added code cleanup datetime ADOdb Date/Time Library labels May 18, 2023
@dregad dregad added this to the v5.23.0 milestone May 18, 2023
@dregad dregad self-assigned this May 18, 2023
@dregad
Copy link
Member Author

dregad commented May 18, 2023

Marked as deprecated in 5.22.6, will be removed in 5.23.0

@dregad dregad closed this as completed in bffa42e May 18, 2023
@dregad
Copy link
Member Author

dregad commented May 18, 2023

Documentation has been updated with warning boxes indicating that the library is deprecated in 5.22.6 and removed in 5.23.0.

@ptdesign
Copy link

adodb_mktime removed unfinished. still adodb_mktime present in adodb.inc.php and adodb-sybase.inc.php

@dregad
Copy link
Member Author

dregad commented Jun 22, 2023

adodb_mktime removed unfinished. still adodb_mktime present in adodb.inc.php and adodb-sybase.inc.php

@ptdesign yes, I noticed that a couple days ago as well. Will fix.

@dregad dregad reopened this Jun 22, 2023
@ptdesign
Copy link

Hello, any updates?

@dregad
Copy link
Member Author

dregad commented Sep 26, 2023

Sorry no, I've been busy and have not been able to work on ADOdb lately... But it's not forgotten and I'll get to it eventually.

@joebordes
Copy link
Contributor

joebordes commented Sep 26, 2023

Can I try to clean that up? Make a PR?

Looking for things to do for this year hacktoberfest. If I can help here would be great!

@dregad
Copy link
Member Author

dregad commented Sep 27, 2023

@joebordes contributions are always welcome, feel free to submit a pull request. Thanks !

@joebordes
Copy link
Contributor

Got it! PR on it's way :-)

@joebordes
Copy link
Contributor

Can you please add the hacktoberfest label to the project?

@dregad
Copy link
Member Author

dregad commented Sep 30, 2023

Can you please add the hacktoberfest label to the project?

Done

joebordes added a commit to joebordes/ADOdb that referenced this issue Sep 30, 2023
@joebordes
Copy link
Contributor

Looking at the code and the commit where the date/time library was eliminated I found that there were 4 functions being used and not defined:

  • adodb_gmdate
  • adodb_date2
  • adodb_date
  • adodb_mktime

I studied a little about the history and the functionality and decided (hopefully correctly) to just use the standard PHP functions.

Then I validated my assumptions by creating an SQLite database and trying to access it. I received the error as expected:

PHP Fatal error:  Uncaught TypeError: SQLite3::createFunction():
 Argument #2 ($callback) must be a valid callback, function "adodb_date" not found or invalid function name
 in /home/joe/reps/ADOdb/drivers/adodb-sqlite3.inc.php:312

I then made the change and created a test file to validate the functionality. This test file is also in the pull request but not integrated into the test suite you have. It is just there as a starting point for future efforts.

$> php testsqlite.php 
Test sqlite access first 4 rows: select * from testtable order by id limit 4  ✓
Test SQLDate: SELECT strftime('%d-%m-%Y Q %H:%M:%S')  ✓
Test SQLDate: SELECT strftime('%d-%m-%Y Q %H:%M:%S','1974-02-25')  ✓
Test SQLDate: SELECT strftime('%d-%m-%Y Q %H:%M:%S','1474-02-25 05:04:16')  ✓
Test SQLDate: SELECT strftime('%d-%m-%Y Q %H:%M:%S','2474-02-25 05:04:16')  ✓

Let me know how it looks and if you need me to change anything.

@dregad
Copy link
Member Author

dregad commented Oct 15, 2023

Hello, sorry for the lack of response, I've been busy...

You mentioned a pull request, but you never actually created one... Anyway I had a quick look at the commit in your fork and it looks good.

My only concern is the continued use of strftime() which has been deprecated in PHP 8.1. I know it's somewhat outside the scope of this issue and I have not analyzed the implications, but maybe this would be an opportunity to consider implementing an alternative (maybe as a 2nd PR).

Thanks

@joebordes
Copy link
Contributor

this one: #1002

@joebordes
Copy link
Contributor

I had a look at that strftime comment but I think it doesn't apply. That specific strftime is not evaluated by PHP but by SQLite: https://www.sqlite.org/lang_datefunc.html

I eliminated the createfunction call and I am using native SQLite.

Let me know

@dregad dregad linked a pull request Oct 15, 2023 that will close this issue
@dregad
Copy link
Member Author

dregad commented Oct 15, 2023

this one: #1002

Sorry I missed it as it was not referenced from here and was looking from my phone. Thanks

That specific strftime is not evaluated by PHP but by SQLite

Right, I missed the quotes when reviewing... Ignore me 😉

dregad pushed a commit that referenced this issue Oct 18, 2023
This is a follow-up on commit bffa42e.

Following removal of adodb-time library, some function calls were not
cleaned up in the code base. This fixes that.

Includes a standalone test script to validate the functionality. 

Fixes #970 (PR #1002)
@dregad
Copy link
Member Author

dregad commented Oct 18, 2023

Many thanks for your contribution

@joebordes
Copy link
Contributor

my pleasure :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup datetime ADOdb Date/Time Library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants