8000 Breaking change with 6.1.0 after 6.0.2 · Issue #2562 · phpredis/phpredis · GitHub
[go: up one dir, main page]
Skip to content

Breaking change with 6.1.0 after 6.0.2 #2562

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.

Sign up for GitHub

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

Jump to bottom
Open
2 tasks done
oprypkhantc opened this issue Oct 7, 2024 · 24 comments
Open
2 tasks done

Breaking change with 6.1.0 after 6.0.2 #2562

oprypkhantc opened this issue Oct 7, 2024 · 24 comments

Comments

@oprypkhantc
Copy link

Expected behaviour

zScan() supports string "0" as the parameter for #2 $it argument:

$cursor = '0';

// Returns cursor and items
[$cursor, $items] = $redis->zScan('laravel:tag:something', $cursor, '*', 10);

Actual behaviour

zScan() returns false when trying to pass the string "0" as the parameter for $it:

$cursor = '0';

// Returns false
[$cursor, $items] = $redis->zScan('laravel:tag:something', $cursor, '*', 10);

I'm seeing this behaviour on

  • OS: Linux
  • Redis: 7.0.7
  • PHP: 8.2.18
  • phpredis: 6.1.0

Steps to reproduce, backtrace or example script

$redis = new \Redis(['host' => 'redis', 'port' => 6379]);
$redis->zAdd('laravel:tag:something', -1, 'value');
$redis->zScan('laravel:tag:something', $cursor, '*', 10);

Prior to 6.1.0, it used to support the string '0' cursor. This behaviour is used in by the current version of Laravel, and is an unexpected breaking change: https://github.com/laravel/framework/blob/5caf767f708aa6e8a3774104ecc7dd0471db805e/src/Illuminate/Cache/RedisStore.php#L298

I've checked

  • There is no similar issue from other users
  • Issue isn't fixed in develop branch
@NiklasBr
Copy link
NiklasBr commented Oct 7, 2024

We are also experiencing other breaking changes which are not semver-friendly such as hset and:

public function exec(): bool|null;

Changed to:

public function exec(): bool|null|array;

It's not wrong to do those changes, but maybe the version should have changed to 7.0 instead of 6.1?

@yatsukhnenko
Copy link
Member

@oprypkhantc in 6.1.0 second parameter may be string

Method [ <internal:redis> public method zscan ] {
      - Parameters [4] {
        Parameter #0 [ <required> string $key ]
        Parameter #1 [ <required> string|int|null &$iterator ]
        Parameter #2 [ <optional> ?string $pattern = null ]
        Parameter #3 [ <optional> int $count = 0 ]
      }
      - Return [ Redis|array|false ]
    }

The problem is in how '0' is handled in new version

@yatsukhnenko
Copy link
Member

@NiklasBr semver allows changes like that for a bug fixes, for example exec always returned array with results of multiple operations but there was a bug in stubs

@yatsukhnenko
Copy link
Member

The problem is in how '0' is handled in new version

Or the problem is in how laravel uses it 😃

@michael-grunder
Copy link
Member
michael-grunder commented Oct 7, 2024 •
edited
Loading

For reference, the change to SCAN is because of #2454. (Fixed via #2458, and #2462).

PHP's int can only represent -2^63 - 2^63 - 1 whereas Redis SCAN cursors are unsigned 64-bit values, so we had to find a solution for very large values.

Edit: What used to happen is we would test if the passed cursor wasn't an int (or was negative) and we would just treat that as the first iteration because non-integer cursors were not legal.

Now we have to allow for string cursors to support very large values as described above.

It should be possible to support the previous behavior in 6.1.1 given that we will never return a string cursor smaller than 2^63 if people think that's worth doing.

@NiklasBr
Copy link
NiklasBr commented Oct 8, 2024

@NiklasBr semver allows changes like that for a bug fixes, for example exec always returned array with results of multiple operations but there was a bug in stubs

Yes, it is allowed. But are you sure it's just the stubs that changed? I'm getting breaking changes with the ->hset() method as implemented in code. There are some mentions of HSET in the changelog, but nothing that spells out that code relying on this need to change at the time of updating.

@oprypkhantc
Copy link
Author

Or the problem is in how laravel uses it 😃

Sure, but I believe they use a string value to support both phpredis and the predis package without additional code. And it kind of makes sense - since the command itself in Redis supports 0 as the initial cursor.

From PHP perspective, null does make more sense, but I wouldn't blame Laravel here :)

@yatsukhnenko
Copy link
Member

I'm getting breaking changes with the ->hset() method as implemented in code.

IMO The problem with hset is ambiguous. If you use PhpRedis without any wrappers/proxy/etc hset call in your code should work fine on both 6.0.X and 6.1.X versions and doesn't break anything.

@CRC-Mismatch
Copy link
CRC-Mismatch commented Oct 8, 2024 •
edited
Loading

I'm getting breaking changes with the ->hset() method as implemented in code.

IMO The problem with hset is ambiguous. If you use PhpRedis without any wrappers/proxy/etc hset call in your code should work fine on both 6.0.X and 6.1.X versions and doesn't break anything.

But still, if my code is overriding hSet, it WILL break with 6.1.0...

And about wrappers/proxy/etc, not only Laravel is affected, but also Symfony... (And I'm willing to bet that also https://github.com/colinmollenhour/credis is affected in the same way, which means Magento is also indirectly affected)
image

The problem at large may be because of PECL's versioning limitations ("current year of Our Lord 2024" and they still only allow pinning specific versions, without any capability of limiting updates to minor versions if you don't want to micro-manage every single extension), but then even more so, when acknowledging this, maintainers of extensions that are mainly available via PECL should have a sense of responsibility whenever they publish breaking changes... (early deprecation warnings are welcome)

This was referenced Oct 9, 2024
Closed
Closed
@jayan-blutui jayan-blutui mentioned this issue Oct 9, 2024
Merged
@AndriusFromLTU
Copy link
AndriusFromLTU commented Oct 15, 2024 •
edited
Loading

A lot of people use Laravel and their caching. This change is breaking so many production systems, that it is not even funny :(

@RikudouSage
Copy link

Also #2564 which was closed because they don't consider breaking changes to be breaking changes.

@Trogie
Copy link
Trogie commented Oct 17, 2024

@myDisconnect if it breaks your production system then that's your mistake to not build from lockfiles in code repository.
We regularly update the lockfiles in feature branches, develop, acpt and before it gets onto production we always find breaking upgrades.

@autaut03
Copy link

@Trogie How to use a lockfile with PECL? We use https://github.com/mlocati/docker-php-extension-installer and the only thing it allows is pinning versions. It's not a lock file, and I'm not aware of any solutions with actual lock files for Docker/PECL.

@Trogie
Copy link
8000
Trogie commented Oct 17, 2024 •
edited
Loading

I was indeed to fast typing: I lock the versions in the Dockerfiles for the different environments.
Developers use that repo here only for localhost. I would never trust that on my environments (dev->...->production).
I build a php base-container only from Official Images or Verified Publishers with individual pecl installs, so Dockerfile just contains this for redis:
RUN pecl install redis-6.0.2
RUN docker-php-ext-enable redis
Yes it takes a little longer to build, but way more secure.

@NiklasBr
Copy link

Using a different Docker image in dev and prod kind of defeats the purpose of building and running those images in the first place; they create a mostly deterministic environment where the local environment is the same as what is deployed.

@Trogie
Copy link
Trogie commented Oct 17, 2024

Your choice, we consider our production environments not breaking a bigger prio then having full deterministic environments.
The diff between production and other env Dockerfiles are only the fixed versions.
We saw this break on dev env, fixed the version for now and dev env was back up after 3 minutes...

@michael-grunder
Copy link
Member

Would it reduce the pain here if we introduced a configure option in 6.1.1. that let you opt-out of the HSET change?

Something like:

./configure --legacy-hset-prototype

It's a terrible hack and I kind of hate it 😄 but the stub generator has full support for something like this via #ifdef blocks.

@NiklasBr
Copy link

Not for my part, we have workarounds now.

I think the best solution would have been to tag the old method as @deprecated Since version X, will be removed in version Y, please use method class::foo() instead, maybe also with an actual warning using \trigger_error("Method " . __METHOD__ . " is deprecated since ...", \E_USER_DEPRECATED);. But I suppose that ship has sailed, and fully restoring the old behaviour will probably cause even more confusion.

Maybe introduce a compatibility layer (which is more work I understand is often a chore), but allow it to accept both the old and the new style input, but trigger a warning on the old?

@shyim shyim mentioned this issue Oct 22, 2024
Open
@tillkruss tillkruss mentioned this issue Nov 8, 2024
Closed
@michael-grunder
Copy link
Member

We're going to put together 6.1.1 soon. Let me know if there's anything I could include to make this less of a pain point. Conditional compilation is likely the only real solution that doesn't break the 6.1.0 BC.

We won't modify any more signatures until 7.0.0 though.

@RikudouSage
Copy link

Since this is an extension, can't the method have multiple signatures? Though I don't know how hard it is to implement. Making the old deprecated using trigger_error is a good idea.

@xabbuh
Copy link
xabbuh commented Nov 12, 2024

Conditional compilation is likely the only real solution that doesn't break the 6.1.0 BC.

From the Symfony POV what we would need then is another way to (easily) detect for which signature we need to adapt. Right now we do that by checking the extension version. That wouldn't work anymore with this idea. Having a constant as an indicator would probably the easiest solution for us.

@HMAZonderland HMAZonderland mentioned this issue Nov 15, 2024
Closed
@michael-grunder
Copy link
Member

Having a constant as an indicator would probably the easiest solution for us

That's easy to do. Maybe something like VARIADIC_HSET? I can name it whatever people prefer.

@SCIF SCIF mentioned this issue Dec 4, 2024
Closed
2 tasks
@SCIF
Copy link
SCIF commented Dec 4, 2024 •
edited
Loading

Hi @yatsukhnenko ,

The problem is in how '0' is handled in new version

Or the problem is in how laravel uses it 😃

Based on the documentation the second argument of hScan() method must have type long, i.e. float. But the example underneath the description uses null as input so I assume the proper type is null|float. However, there is no strict type checking so a cursor of any scalar type (at least) could be supplied. The problem is that 0.0 is affected in the same way as '0'

@bentleyo bentleyo mentioned this issue Dec 12, 2024
Closed
1 task
@SCIF
Copy link
SCIF commented Dec 19, 2024 •
edited
Loading

Hi @michael-grunder ,

For reference, the change to SCAN is because of #2454. (Fixed via #2458, and #2462).

PHP's int can only represent -2^63 - 2^63 - 1 whereas Redis SCAN cursors are unsigned 64-bit values, so we had to find a solution for very large values.

Edit: What used to happen is we would test if the passed cursor wasn't an int (or was negative) and we would just treat that as the first iteration because non-integer cursors were not legal.

I created an issue (which I treated duplicate and closed immediately after creation) showing that passing 0.0 doesn't work either as initial value for 2nd parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0