[12.x] Improve AggregateServiceProvider docblocks

[cosmastech]

Nothing major here, just some more specific docblocks.

[AhmedAlaa4611]

Hello @cosmastech

Since you've been making a lot of great updates in this area, would you mind also taking a quick look at the docblock for:

framework/src/Illuminate/Cache/RedisStore.php

Line 449 in a46b551

* @param mixed $value

framework/src/Illuminate/Cache/RedisStore.php

Line 460 in a46b551

* @param mixed $value

I think it might be inaccurate, and your input would be really helpful.

[cosmastech]

Hello @cosmastech

Since you've been making a lot of great updates in this area, would you mind also taking a quick look at the docblock for:

framework/src/Illuminate/Cache/RedisStore.php

Line 449 in a46b551

* @param mixed $value

framework/src/Illuminate/Cache/RedisStore.php

Line 460 in a46b551

* @param mixed $value

I think it might be inaccurate, and your input would be really helpful.

Thanks for reaching out @AhmedAlaa4611. Let's take a look.

/**
 * Determine if the given value should be stored as plain value.
 *
 * @param  mixed  $value
 * @return bool
 */
protected function shouldBeStoredWithoutSerialization($value): bool
{
    return is_numeric($value) && ! in_array($value, [INF, -INF]) && ! is_nan($value);
}

is_numeric() accepts mixed (an int, float, or a numeric string), while is_nan() accepts a float (or something that can be cast to a float, like the string '1.1e10'). I think that mixed here would be appropriate. Perhaps there's something clever that can be done with conditional return types, though I'm not confident I know what that is.

The return type is definitely bool, as it is the result of a boolean expression.

Our conditions tell us:

• is_numeric() assures we have a value that can be cast to a float or int.
• that value is not INF or -INF or NAN (specials constants representing floats)

/**
 * Serialize the value.
 *
 * @param  mixed  $value
 * @return mixed
 */
protected function serialize($value)
{
    return $this->shouldBeStoredWithoutSerialization($value) ? $value : serialize($value);
}

This one is a little bit more interesting. From the above, we know that in order to return an unserialized numeric value here, we must have a number or numeric string for our value. Otherwise, we just return the result of serialize() which is always a string.

I think that given we have to be loose enough to allow any input $value, our only possible optimization would be to narrow the return type. In this case, that would be number|numeric-string|string.

Some would make the case that adding numeric-string and string is redundant. I do not fall into that camp myself, but I see that there was recently a large MR that dropped all returns types except for mixed if mixed was an option.

Again here, you could maybe do something clever with conditional return types. (Feel like @calebdw would probably have some better insight on this, as I rarely write conditional return statements).

Additionally

I think that is_finite() could replace ! in_array($value, [INF, -INF]) && ! is_nan($value). Whether or not that's a meaningful change would be up to the maintainers. 😄

[shaedrich]

/**
 * Serialize the value.
 *
 * @param  mixed  $value
 * @return mixed
 */
protected function serialize($value)
{
    return $this->shouldBeStoredWithoutSerialization($value) ? $value : serialize($value);
}

This one is a little bit more interesting. From the above, we know that in order to return an unserialized numeric value here, we must have a number or numeric string for our value. Otherwise, we just return the result of serialize() which is always a string.

Here, a combination of generics and a conditional return type would be quite effective.

Furthermore, shouldBeStoredWithoutSerialization() might benefit from an assert annotation:

/**
 * Determine if the given value should be stored as plain value.
 *
 * @param  mixed  $value
 * @return bool
 * @phpstan-assert-if-true numeric-string|float $value
 * @phpstan-assert-if-false NAN|INF|-INF $value
 */
protected function shouldBeStoredWithoutSerialization($value): bool
{
    return is_numeric($value) && ! in_array($value, [INF, -INF]) && ! is_nan($value);
}

However, -INF currently doesn't seem to work (see #13477):

PHPDoc tag @phpstan-assert-if-false has invalid value (NAN|INF|-INF $value): Unexpected token "-INF", expected type at offset 210 on line 7

[calebdw]

Honestly, that seems more complicated than necessary. Both of these methods are protected and the only location that shouldBeStoredWithoutSerialization is used is in the serialize method.

If anything, you might update the serialize return to int|float|string, but this has no benefit to the end user

[AhmedAlaa4611]

@cosmastech @shaedrich @calebdw

Thank you all for the explanation.