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 serialization of ComparisionFailure with PDO in stacktrace. #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DomBlack
Copy link

@DomBlack DomBlack commented Mar 8, 2018

This fix implements a custom serialize/deserialize method on the
ComparisionFailure class, which means it loses the stack trace
during serialization.

This allows PHPUnit to run in process isolation mode when the stack
trace may contain references to non-serializable objects such as
the PDO.

This fix implements a custom serialize/deserialize method on the
ComparisionFailure class, which means it loses the stack trace
during serialization.

This allows PHPUnit to run in process isolation mode when the stack
trace may contain references to non-serializable objects such as
the PDO.
@codecov-io
Copy link

codecov-io commented Sep 27, 2018

Codecov Report

Merging #47 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #47      +/-   ##
============================================
+ Coverage     99.07%   99.09%   +0.02%     
- Complexity      123      125       +2     
============================================
  Files            15       15              
  Lines           325      333       +8     
============================================
+ Hits            322      330       +8     
  Misses            3        3
Impacted Files Coverage Δ Complexity Δ
src/ComparisonFailure.php 100% <100%> (ø) 11 <2> (+2) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2256ef8...3fdfacd. Read the comment docs.

@DomBlack
Copy link
Author

@sebastianbergmann I've re based on the current master branch. Is there any chance of getting this merged?

Also I think the codecov-io report is for the previous version against the current master; rather than the current version of this PR

@mpyw
Copy link

mpyw commented Jun 23, 2021

@DomBlack The problem still exists so I wrote the patch: mpyw/phpunit-patch-serializable-comparison

@mdlutz24
Copy link

mdlutz24 commented Jul 3, 2022

@sebastianbergmann @DomBlack We've been running into this issue in the Drupal community as well. I see this PR does need an update to use the __serialize and __unserialize magic methods introduced in php 7.4, rather than implementing the Serializable interface. If we can get an updated PR, I'd like to know the chances of being able to get it merged, as we could definitely use it downstream.

Related Drupal Core issue: https://www.drupal.org/project/drupal/issues/3197324

@sebastianbergmann
Copy link
Owner

The only thing I can promise that I would review such a pull request.

@twoseascharlie
Copy link

twoseascharlie commented Jun 8, 2023

I am running into similar issue as well with having object as args on the stack trace that cannot be serialized when running in isolation.

Which is related to issue: sebastianbergmann/phpunit/issues/1351 and pull: sebastianbergmann/phpunit/issues/1352

In that PR, instead of trying to override serialization methods, the constructor just unset the args on each stack element:
see commit for Framework/Exception.php

Relevant code from Framework/Exception.php in that PR:

    public function __construct($message = '', $code = 0, Exception $previous = null)
    {
        parent::__construct($message, $code, $previous);

        $this->serializableTrace = $this->getTrace();
        foreach ($this->serializableTrace as $i => $call) {
            unset($this->serializableTrace[$i]['args']);
        }
    }

maybe a similar approach could be utilized in ComparisonFailure?

-- edit/update to my own message...
After digging into this one a bit more, seems that overriding the serialization methods probably is the best route, just leaving my original comment for additional information.

@Boegie
Copy link

Boegie commented Jul 1, 2023

Added a (very naive) first stab at updating the old PR in #106.

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.

7 participants