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: try using __serialize when obj implements \Serializable #567

Merged

Conversation

pcoutinho
Copy link

Description of the change

The Serializable interface is now being deprecated in favour of using
the magic methods __serialize() and __unserialize().

Some packages decided to start throwing exceptions on their Serializable
implementations if the interface methods were called.

This commit tries to check if the classes that implement Serializable
are already using __serialize() and if they are, it calls that method,
instead of the interface one serialize().

If this is the case, we also hide the deprecation notice, since it seems
the client code is already preparing to drop \Serializable when the time
comes.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Related issues

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

The Serializable interface is now being deprecated in favour of using
the magic methods __serialize() and __unserialize().

Some packages decided to start throwing exceptions on their Serializable
implementations if the interface methods were called.

This commit tries to check if the classes that implement Serializable
are already using __serialize() and if they are, it calls that method,
instead of the interface one serialize().

If this is the case, we also hide the deprecation notice, since it seems
the client code is already preparing to drop \Serializable when the time
comes.

This should fix issue rollbar#136 on  rollbar/rollbar-php-laravel
@danielmorell
Copy link
Collaborator

This initially looks like a good direction to take things. I am going to dig into it more this week.

@desi
Copy link

desi commented Apr 28, 2022

Hello all! Just wanted to pop over here and let you all know we are watching this PR and waiting for it to come through as we need this on our team as well! Thank you for all that you do!

@L1lle
Copy link

L1lle commented May 2, 2022

I can not use Rollbar on any of my Laravel 9 projects at the moment. I'm running blind for over a month now, checking logs once a day manually. I don't want to be pushy, but is there any timeline on this issue?

@jamesaspence
Copy link

Hi all, we are in the same boat - our rollbar integration has been disabled as we eagerly await this fix. Any chance of this fix making it through approval and out this week? Anything we can do to help speed along the fix?

@dimkax94
Copy link

dimkax94 commented May 5, 2022

Same for me. Had to use the forked version to avoid this issue

@jamesaspence
Copy link

CC @danielmorell it's been two weeks since this PR was created, any update on your end? There are a handful of us who are currently stuck without good monitoring while we await this fix.

@pcoutinho
Copy link
Author

pcoutinho commented May 11, 2022

I'm not sure if this fix is up to Rollbar's standards, but it would be great if we could get the ball moving on this, hence the PR.

I'm currently running some Laravel applications with Bref on Lambda and if an uncaught exception is thrown my lambdas are stuck on an infinite loop trying to process unhandled exceptions, until Lambda shuts it down. It's a real pain.

Note that I do understand that this was a decision taken by Symfony to move people away from using the Serializable interface.

@jonnott
Copy link

jonnott commented May 11, 2022

I'm pretty much on the cusp of ditching rollbar in favour of bugsnag at this point. Anyone else..?

@cyrusradfar
Copy link

cyrusradfar commented May 11, 2022

@jonnott @jamesaspence et al... hey folks, I won't speak publicly about personal things but @danielmorell had great reason to miss this follow up. I'm watching and we'll push this through. We'll need to find a better way of escalating these lagging issues to our team.

If you have suggestions or have experience managing dozens of public repos, I'm open to feedback on tools or integrations.

Copy link
Collaborator

@danielmorell danielmorell left a comment

Choose a reason for hiding this comment

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

I think this is a good step in the right direction. Thank you @pcoutinho for your work on this! Sorry for the delay!

@danielmorell danielmorell merged commit 32fdf9f into rollbar:master May 11, 2022
@danielmorell
Copy link
Collaborator

Hey everyone, this has been released in v3.1.3-RC1. Please test it out and provide any feedback you have. If all looks good, we will release the stable v3.1.3.

@roelVerdonschot
Copy link

@danielmorell Can you update the rollbar-php-laravel package to use this release

@danielmorell
Copy link
Collaborator

danielmorell commented May 13, 2022

@roelVerdonschot This is a pre-release version so that would not be wise. You can install it by running...

$ composer config prefer-stable false && composer update rollbar/rollbar

@jamesaspence
Copy link

@danielmorell any rough ETA on when rollbar-php-laravel will be updated with this fixed version?

@danielmorell
Copy link
Collaborator

Hey @jamesaspence, you could install it by running...

$ composer config prefer-stable false && composer update rollbar/rollbar && composer config prefer-stable true

Please give us any feedback you have on whether it has any issues, so we can make the determination about a stable release. Without any feedback we will release v3.1.3 on 2022/05/30. At that point you can update to the latest version by simply running...

$ composer update rollbar/rollbar

@jamesaspence
Copy link

Looks like the bugfix rolled out today - I was able to retrieve it by bumping my rollbar-laravel dependency (which pulled in v3.1.13). I'll be testing it in the next few days!

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