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

Backward compatible usage of the new __serialize/__deserialize methods #105

Merged
merged 2 commits into from Nov 15, 2021

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented Nov 13, 2021

Q A
Bug fix? no
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT

This was referenced Nov 13, 2021
@goetas
Copy link
Collaborator Author

goetas commented Nov 13, 2021

@alexander-schranz @W0rma could you please try this?

@ruudk
Copy link
Contributor

ruudk commented Nov 15, 2021

I also noticed this problem on PHP 8.1 and was waiting for a fix. I just tried out this branch, it did solve the problem.

With the previous fix, it would only work when there was no cache, but as soon as it was read from cache it would break. Now it works as expected 🎉

@goetas goetas merged commit 3c5f34f into master Nov 15, 2021
@goetas goetas deleted the backward-compat-8.1 branch November 15, 2021 12:06
@ruudk
Copy link
Contributor

ruudk commented Nov 15, 2021

@goetas Thanks for merging this. It would be great to have this tagged so that I can continue testing SimpleBus on PHP 8.1 🙏

@goetas
Copy link
Collaborator Author

goetas commented Nov 15, 2021

@wachterjohannes
Copy link

@goetas we have "tested" your new release (2.5.1 green 2.5.2 red) thru our github workflow (https://github.com/sulu/sulu/runs/4222298775?check_suite_focus=true) and it seems to have a bug which causes a segfault during the serialization process. the segfault happens inside of this function https://github.com/massiveart/MassiveSearchBundle/blob/2.x/Search/Metadata/ClassMetadata.php#L83 . do you have any idea what the problem could be?

@goetas
Copy link
Collaborator Author

goetas commented Nov 16, 2021

hmm a segfault... that is scary :( , no idea :(

@alexander-schranz
Copy link
Contributor

The segmentation fault, seems also happening here: https://github.com/massiveart/MassiveSearchBundle/runs/4265624083?check_suite_focus=true

@alexander-schranz
Copy link
Contributor

@goetas it also very strange on 3v4l it ends in a memory limit: https://3v4l.org/CmKHo

$this->class,
$this->name,
];
return [$this->serialize()];
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to trigger the parent $this->serialize method which will then end in a segmentation fault. Instead $this->serialize I think we need to go with implementing it seperatly. Still this would crash applicatons not implementing __serialize and __unserialize method so I would still suggest great a new major release and revert this changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if we do a major release there is no way to allow applications to support both v2 and v3 versions of this library.

while with this implementation people can just add to their code

    public function __serialize(): array
    {
        return [$this->serialize()];
    }

    public function __unserialize(array $data): void
    {
        $this->unserialize($data[0]);
    }

and they got it working, or if they do not care about deprecations, it works anyway even without adding this code.

@goetas
Copy link
Collaborator Author

goetas commented Nov 19, 2021

@alexander-schranz your custom methods are wrong:

    public function __serialize(): array
    {
        return $this->serialize();
    }

    public function __unserialize(array $data): void
    {
        $this->unserialize($data);
    }

should https://3v4l.org/ll2IK

@alexander-schranz
Copy link
Contributor

@goetas still the same result it ends then in a segmentation fault: massiveart/MassiveSearchBundle#162

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.

None yet

4 participants