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

Numerous drivers make unneeded PHP4 version checks #584

Closed
mnewnham opened this issue Jan 5, 2020 · 6 comments · Fixed by #588
Closed

Numerous drivers make unneeded PHP4 version checks #584

mnewnham opened this issue Jan 5, 2020 · 6 comments · Fixed by #588
Assignees
Labels
code cleanup core ADOdb core (library and base classes)
Milestone

Comments

@mnewnham
Copy link
Contributor

mnewnham commented Jan 5, 2020

The following drivers check for PHP version 4.0.5 and higher
ads, firebird, ibase, mssl, mssqlnative, mysql, oci8, oci8po odbc, postgres64, sybase

@mnewnham mnewnham added code cleanup core ADOdb core (library and base classes) labels Jan 5, 2020
@mnewnham mnewnham self-assigned this Jan 5, 2020
@dregad
Copy link
Member

dregad commented Jan 5, 2020

@mnewnham Great minds think alike ! I noticed some occurences of this yesterday, and added this to my todo list too 😃 Since you assigned the issue to yourself I assume you're working on it, so I'll wait for your patch.

I would suggest to get rid of the ADODB_PHPVER constant (defined in adodb.inc.php lines 181-190) entirely, and just leave a check to die if PHP < 5.0.

Note that there are also several checks on PHP_VERSION, which should be reviewed as part of this work item too.

@dregad dregad added this to the v5.21 milestone Jan 5, 2020
@mnewnham
Copy link
Contributor Author

mnewnham commented Jan 6, 2020

Could you take a look at the mssql driver for me, the way I read it is that if the php version > 4.3 then the global ''ADODB_mssql_mths'' is never declared correctly. If the inbound date passed to unixDate() was in the format "M d y", then the function would crash.

Either:
a. I'm not reading it correctly
b. Its never been used like that and we should drop the special handling for this driver and make it consistent with the others which would allow us to also remove the AutoDetect_MSSQL_Date_Order() function as well (incidentally I never knew this function existed before, definitely not documented).

@dregad
Copy link
Member

dregad commented Jan 6, 2020

the global ''ADODB_mssql_mths'' is never declared correctly

In this context, the global $ADODB_mssql_mths; is needed in the else block so it can be initialized. It is also declared everywhere the constant is used so there wouldn't be any scope problem here.

That being said, I don't know enough about MSSQL and how the drivers work in later versions of PHP to be able to reply with absolute certainty, but it looks like this whole $ADODB_mssql_xxx is only needed for PHP <4.3.

@mnewnham
Copy link
Contributor Author

mnewnham commented Jan 7, 2020

In that case I'm going to strip all of it out.

@mnewnham
Copy link
Contributor Author

mnewnham commented Jan 7, 2020

I've just seen This. I was not aware of this (having migrated my own stuff to mssqlnative years ago), but I thought that it was still active. I would say then deprecate mssql in 5.21. I'm going to open a new thread about removing a whole bunch of drivers in 5.22 and I'll add that to the list

@dregad
Copy link
Member

dregad commented Jan 7, 2020

I've just seen This. I was not aware of this

Didn't notice either, as I don't use MSSQL at all...

deprecate mssql in 5.21. I'm going to open a new thread about removing a whole bunch of drivers in 5.22 and I'll add that to the list

Sounds good.

I'm starting to think that with all the removed stuff, maybe the next release should be 6.0, and include namespacing and refactoring of the library into individual classes (one per file), to be PSR4 compliant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup core ADOdb core (library and base classes)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants