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.

Sign up for GitHub

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

Jump to bottom

Apply fixes to have DB package support PHP7.2 #207

Merged
merged 1 commit into from
May 22, 2018
Merged

Apply fixes to have DB package support PHP7.2 #207

merged 1 commit into from
May 22, 2018

Conversation

seamuslee001
Copy link
Contributor

ping @Jazz-Man @totten this is my alternate and i think it makes more sense given i think by this point $dsn is either NULL or a string

@monishdeb
Copy link
Member

monishdeb commented May 19, 2018 •
edited
Loading

@seamuslee001 when I compared with a PR https://github.com/pear/DB/pull/5/files which brings the PHP7.2 fixes in Pear DB which makes sense too me as I gone through the code. Also there might be possibility to get null $dsn (as per L780) and thus strlen check will fail on empty string, so can you change it to if(empty($dsn)) ?

@seamuslee001
Update DB package to support PHP7.2
2bd5e33 
switch count to strlen when parasing the DSN

apply fixes from upstream PR
@seamuslee001 seamuslee001 changed the title switch count to strlen when parasing the DSN Apply fixes to have DB package support PHP7.2 May 19, 2018
@seamuslee001
Copy link
Contributor Author

@monishdeb updated and applied the other fixes from the upstream PR

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

monishdeb
monishdeb approved these changes May 20, 2018
View reviewed changes
Copy link
Member

@monishdeb monishdeb left a comment

Choose a reason for hiding this comment

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

The latest patch worked for me. Tested with different $dsn format w/o patch and in both case return same result. Also reviewed the code and confirmed that it doesn't cause any unintended regression.

@monishdeb monishdeb merged commit d619e82 into civicrm:master May 22, 2018
@christianwach
Copy link
Member

This works for me too.

@seamuslee001 seamuslee001 deleted the string_dsn branch June 26, 2018 21:01
@MegaphoneJon MegaphoneJon mentioned this pull request Aug 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monishdeb monishdeb monishdeb approved these changes

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seamuslee001 @monishdeb @christianwach @civicrm-builder