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

Fix PHP4 style constructs #117

Merged
merged 4 commits into from
Feb 23, 2017
Merged

Fix PHP4 style constructs #117

merged 4 commits into from
Feb 23, 2017

Conversation

Scott-Mc
Copy link

Fixing the old PHP4 style constructs across the various drivers for PHP7 comparability.

I left the mysql and mssql drivers as is given these are both depreciated.

This was referenced Feb 17, 2017
@jv2222
Copy link
Contributor

jv2222 commented Feb 17, 2017

This is an interesting scenario. A lot of good (I think) changes here but the lib is so old and no test suite. Also, I haven't personally worked with the library for a few years. I could just click merge but I'm not 100% sure that it won't cause issues.

In a bit of a pickle and not sure how to move forward!

I guess we could go through each change line by line and discuss with @gnowxilef and other maintainers.

I think this really points to the need for a unit test suite but I don't have the bandwidth for that :(

One question @Scott-Mc Do all the demos work out of the box with these changes?

@Scott-Mc
Copy link
Author

I've only been able to test mysqli, sqlite and postgres and all seem fine on both PHP 5.6 and PHP 7 The bulk of the changes is just moving from the old way to declare constructs to avoid logs being flooded. I intentionally left out the mysql/mssql driver given these are depreciated in newer versions so wouldn't work anyway for those still running the old version.

I've also tested the set names and this works perfectly fine.

A tested suite would be nice

@Scott-Mc Scott-Mc closed this Feb 17, 2017
@Scott-Mc Scott-Mc reopened this Feb 17, 2017
@jv2222
Copy link
Contributor

jv2222 commented Feb 17, 2017

@gnowxilef What do you recommend here?

@fawaf
Copy link
Member

fawaf commented Feb 18, 2017

i agree with @jv2222. it would be great to have a test suite, but that is a big undertaking. (i think that is already a github issue.) it seems like it was tested to the best possible, so i'm fine with it.

@@ -161,7 +161,7 @@ function select($dbname='', $encoding='')
$charsets[] = $row["Charset"];
}
if(in_array($encoding,$charsets)){
$this->dbh->query("SET NAMES '".$encoding."'");
$this->dbh->set_charset($encoding);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one question though: where is set_charset defined?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fawaf
Copy link
Member

fawaf commented Feb 18, 2017

fixes #102

@jv2222 jv2222 merged commit 778f000 into ezSQL:master Feb 23, 2017
@fawaf
Copy link
Member

fawaf commented Feb 24, 2017

should also fix #113

@surruk51
Copy link

surruk51 commented Mar 29, 2017

There are a lot of vars in the orginal - which imply public access on variables that clearly are really for internal use only. There is also the opportunity to catch buggy input in __set and __get routines. There are opportunities to make it better, not just to change the syntax to the PHP5/7 way of making classes. But I guess this would be a fork.

I recast it for PHP5 and 7 for EzSQL_PDO only and wrote a test program but it is not unit testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants