Skip to content

Conversation

rjnicolai
Copy link

Summary

Adjust the processConfigValue check for null, to check on empty. Sometimes the value is false which causes the unserialize method to fail.

Result

If empty() evaluates the code will now return to prevent an exception from being triggered because serializer->unserialize() can't process false.

Checklist

  • I've ran composer run codestyle
  • I've ran composer run analyse

public function processConfigValue(mixed $value, array $config): mixed
{
if ($value === null) {
if (empty($value)) {
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to move this check to the specific array type handler (perhaps extract that to a function)
checking empty here will result in the bool config type not functioning as expected.

Since setting that value to false will now always result in null being returned.

Copy link

@sambolek sambolek Jul 12, 2025

Choose a reason for hiding this comment

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

suggestion:

public function processConfigValue(mixed $value, array $config): mixed
{
    if ($value === null) {
        return null;
    }

    return match ($config['type']) {
        'array' => match (true) {
            is_array($value) => $value,
            is_string($value) && $value !== '' => $this->serializer->unserialize($value),
            default => [],
        },
        'int'    => (int) $value,
        'float'  => (float) $value,
        'bool'   => (bool) $value,
        'string' => (string) $value,
        default  => $value,
    };
}

Choose a reason for hiding this comment

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

For us, the issue was with "logrocket_key" - are we sure this line is correct and that the key should be an array?

'logrocket_key' => ['type' => 'array'],

Copy link
Member

@indykoning indykoning Jul 14, 2025

Choose a reason for hiding this comment

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

You're right, that should indeed not be an array but a string instead.
I'll update an release the change to set that right. After that, feel free to update or close the PR. I Like your suggestion!

Released in 4.4.0 🚀

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.

3 participants