Skip to content

Conversation

bastienho
Copy link
Contributor

required for Serializable since PHP 8.1.0

https://www.php.net/manual/en/class.serializable.php

required for Serializable since PHP 8.1.0
@civibot civibot bot added the master label Oct 13, 2023
@eileenmcnaughton
Copy link
Owner

@bastienho can we just call the existing serialize & unserialize functions - that clean up function for class removes some stuff that doesnt serialize well/ blows out the size / may have sensitive info

@bastienho
Copy link
Contributor Author

@eileenmcnaughton this was my first thought, but serialize() returns a string and _serialize() HAVE TO return an array.

@eileenmcnaughton
Copy link
Owner

@bastienho ok - so we should probably do the same things the cleanup function does in that function too - slightly differently as we would perhaps take get_object_vars & filter stuff out

    if (\Civi::settings()->get('omnipay_developer_mode') && !empty($this->history)) {
      $this->logHttpTraffic(FALSE);
    }
    $this->history = [];
    $this->client = NULL;
    $this->lock = NULL;
    $this->guzzleClient = NULL;
    if ($isIncludeGateWay) {
      $this->gateway = NULL;
    }

@bastienho
Copy link
Contributor Author

I Agree

Should I implement this code in my PR ?

@eileenmcnaughton
Copy link
Owner

@bastienho that would be great

@bastienho
Copy link
Contributor Author

After some tests, we can keep the same behaviour in both functions.

So I suggest to implement the cleanup function in __serialize() and call it in serialize()

@bastienho
Copy link
Contributor Author

@eileenmcnaughton, based on your thougths, I've revamped by PR.

Specially, the implementation of cleanupObjectForSerialization() is not insignificant.

@eileenmcnaughton
Copy link
Owner

thanks @bastienho I'll take a look - I've tagged a release in the meantime to get the regression fix out

@eileenmcnaughton eileenmcnaughton force-pushed the master branch 3 times, most recently from 8f31154 to 679df0f Compare December 21, 2023 19:43
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.

2 participants