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

[REF][PHP8.2] Add patches to pear upstream packages to fix issues with PHP8.2 #24857

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Oct 29, 2022

Overview

This applies some patches / fixes based on upstream PRs to pear/db and pear/core-minimal packages to fix issues in PHP8.2

Before

Pear db and pear core minimal code not compatible for PHP8.2

After

Code seems to work fine for PHP8.2

ping @eileenmcnaughton @demeritcowboy @totten

@civibot
Copy link

civibot bot commented Oct 29, 2022

(Standard links)

@civibot civibot bot added the master label Oct 29, 2022
@demeritcowboy
Copy link
Contributor

It looks like there are some fails on the upstream patch?

@seamuslee001
Copy link
Contributor Author

@demeritcowboy I think the Upstream test fails are unrelated

@seamuslee001 seamuslee001 changed the title Db patches php82 [REF][PHP8.2] Add patches to pear upstream packages to fix issues with PHP8.2 Oct 30, 2022
@demeritcowboy
Copy link
Contributor

Ok I see they've had those errors for a while.
On the db_dataobject one, is the idea it's better to patch DB than DataObject because DataObject is abandoned? Because it looks like that num_rows is never read anywhere.

Otherwise this looks good. And there shouldn't be the same issue as zetamail when they're merged upstream since the version here is fixed at a specific version. Although given this is for php 8.2 would it be easier to just wait until they are merged upstream and then update the pear::db version?

@seamuslee001
Copy link
Contributor Author

@demeritcowboy well I chose to patch DB because that was the class that the dynamic property was being created on when you look at L 4778 http://svn.php.net/viewvc/pear/packages/DB_DataObject/trunk/DB/DataObject.php?view=markup&pathrev=339312

it does this

4776	        // this is a huge bug in DB!
4777	        if (isset($_DB_DATAOBJECT['CONNECTIONS'][$this->_database_dsn_md5])) {
4778	            $_DB_DATAOBJECT['CONNECTIONS'][$this->_database_dsn_md5]->num_rows = array();
4779	        }

Which I think means that we have to patch DB not the dataobject but I suppose we could rip those parts of the DB_DATAObject our of our version instead.

…h PHP8.2

Remove Pear/DB DB_DataObject patch as not needed
@seamuslee001
Copy link
Contributor Author

@demeritcowboy @totten as per discussion here https://lab.civicrm.org/dev/core/-/issues/3958#note_84022 I have pulled the DataObject compatibility patch from this PR and created civicrm/civicrm-packages#354

@demeritcowboy demeritcowboy merged commit c448d32 into civicrm:master Nov 1, 2022
@demeritcowboy
Copy link
Contributor

I clicked around a bit. I figure something would fail pretty quickly if it was a problem.

@eileenmcnaughton eileenmcnaughton deleted the db_patches_php82 branch November 1, 2022 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants