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

[6.x] Support for serializers in PhpRedis connection #30004

Closed
wants to merge 2 commits into from
Closed

[6.x] Support for serializers in PhpRedis connection #30004

wants to merge 2 commits into from

Conversation

ReDev1L
Copy link

@ReDev1L ReDev1L commented Sep 16, 2019

Added support for "none", "php", "igbinary", "msgpack", "json" serializers configuration for PhpRedis connection.

Added support for "none", "php", "igbinary", "msgpack", "json" serializers configuration support for PhpRedis connection
@GrahamCampbell GrahamCampbell changed the title Support for serializers in PhpRedis connection [6.x] Support for serializers in PhpRedis connection Sep 16, 2019
@ReDev1L
Copy link
Author

ReDev1L commented Sep 16, 2019

Will need to update documentation, to include 'serializers' section in config database.php:

'redis' => [

        'client' => env('REDIS_CLIENT', 'phpredis'),

        'options' => [
            'prefix'  => Str::slug(env('REDIS_KEYS_PREFIX', 'land-objects'), '_'),
            'serializer' => Redis::SERIALIZER_IGBINARY,
            // OR
            'serializer' => 'igbinary', // for easier setting of CI vars
        ],

Full available list of values:

  • none

  • php

  • igbinary

  • msgpack

  • json

  • Redis::SERIALIZER_NONE

  • Redis::SERIALIZER_PHP

  • Redis::SERIALIZER_IGBINARY

  • Redis::SERIALIZER_MSGPACK

  • Redis::SERIALIZER_JSON

@GrahamCampbell
Copy link
Member

Maybe we should just have a standard list in Laravel, that converts to the correct stuff at runtime?

@GrahamCampbell
Copy link
Member

Yeh, I really don't like having all those options that are system dependent. We should just support the textual options, and convert to the constants behind the scenes if we need to.

@ReDev1L
Copy link
Author

ReDev1L commented Sep 16, 2019

We can skip in_array check for constants. But better to remain defined checks, so user will get error if he doesnt has proper serializer, that he used in config. If we skip them - there is no good informative way to detect error, phpredis extesion will remain silent, if serializer specified is not available.
Only way is to dump phpredis ext client

@GrahamCampbell
Copy link
Member

I'm not suggesting that. I'm suggesting we don't allow the user to write any of these, and we define our own for the purposes of the Laravel config file. It's then up to Laravel to decide how to map them. That way, we can keep the config file independent of the specific PHP setup.

@netpok
Copy link
Contributor

netpok commented Sep 17, 2019

I would still leave the option to the user, we could define a default (if it worth over the current implementation), but leave the option. Like we don't try to decide which redis client we use, we provide a sensible default.

Of course if by definition igbinary > msgpack > ... > none, then it can be just used that way and all above is needless.

@ReDev1L
Copy link
Author

ReDev1L commented Sep 17, 2019

php-redis extension will use "php" as default, if serializer is not set.

Of course if by definition igbinary > msgpack > ... > none, then it can be just used that way and all above is needless.

That one is dangerous, there can be "microservices" that use one redis instance, but have different php extensions installed. Retarded case, but still possible =).

@DarkGhostHunter
Copy link
Contributor

Is this compatible with serializing engine per redis instance?

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

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.

5 participants