PHPC-2412: Deprecate CursorId class

[alcaeus]

PHPC-2412

The CursorId class was only introduced to hold the 64-bit placeholder. Since The MongoDB\BSON\Int64 class is now more viable, it makes sense to always return a 64-bit value instead of a wrapper class. Note that we could directly return a long on 64-bit systems, but I opted against this in favour of a consistent return type regardless of platform.

This PR will be followed up by a PR in PHPLIB that updates classes implementing CursorInterface or wrapping a cursor. However, as the getId method is not used in the PHPLIB test suite, I don't expect this PR to break any tests there with the new deprecation notice.

[GromNaN]

Note that if $cursor->getId(true) return null, this condition null == 0 would be true. Should you test the type of the returned value?

[alcaeus]

Good point. I'll double check this, as it may have been related to the casting issues fixed in #1617.

[jmikola]

Looking at PHP_METHOD(MongoDB_Driver_Cursor, getId) above, would Cursor::getId() ever return null? It looks like the return value would either be a CursorId or Int64.

The return type in Cursor.stub.php also wouldn't allow a null value (at least for PHP 8.0+).

[alcaeus]

TL;DR: this is not an issue, because getId() would never return null, and even if it did the comparison logic we rely on for Int64 instances handles this correctly.

The dirty details: before the change to getId(true), we'd always get a CursorId instance, which does not implement cast or comparison handlers, so the only sensible way to assert on it was by casting it to a string and doing a typesafe comparison.

Now that we return an Int64 instance, we can rely on PHP's internal logic for checking equality (not identity) in objects. Note that when using ===, there's an early exit in zend_is_identical if the types do not match. So $int64 === 0 will always evaluate to false based on the different types. == on the other hand calls zend_is_equal, which in turn calls zend_compare, which relies on an object's compare handler, and thus defers to the logic we implemented for comparing Int64 instances with other values (and importantly does not include a cast to avoid issues on 32-bit platforms).

I did not add a specific test for the case you mentioned, as zend_compare does not call into the compare handler when comparing an object to null, instead handling it specifically to ensure correct behaviour.

[jmikola]

I noted that this adds ZEND_ACC_DEPRECATED to the CursorId ce_flags. I'm not sure what effect, if any, that has.

Looking at php-src, I only see it used on constants and functions. That agrees with this internals thread related to the #[Deprecated] attribute RFC.

I think the ce_flags change is an unintended side effect of getFlagsAsArginfoString() in gen_stub.php. Should we remove this?

[alcaeus]

Good point. Looking into zend_compile.h, ZEND_ACC_DEPRECATED shares a value with ZEND_ACC_PROTECTED_SET (used as property flag only for asymmetric visibility), and ZEND_ACC_USE_GUARDS (set on the class when any __get, __set, __isset, or __unset are implemented by a class). Since ZEND_ACC_DEPRECATED is defined as a function flag and its value conflicts with a class flag with an entirely different meaning, I've removed the @deprecated comment. This seemed easier than teaching gen_stub.php that @deprecated on a class should not result in ZEND_ACC_DEPRECATED being added to ce_flags. I'll still follow up on whether gen_stub.php should be changed to account for this.

[jmikola]

Ah, I didn't think to check whether ZEND_ACC_DEPRECATED conflicted with actual class flags. I'm glad we caught this then. It does seem like something gen_stub.php should probably be careful to avoid.

[jmikola]

Is this worth using a use statement, or do those not work in stubs? I feel like we've discussed this before and there's probably a reason none of the existing stubs have use statements.

[alcaeus]

Unfortunately, gen_stub.php does not support this (at least in version 8.2 which I currently use to regenerate arginfo files):

❯❯❯ ./build/gen_stub.php
In ./src/MongoDB/Cursor.stub.php:
Unexpected node Stmt_Use

[jmikola]

Looking at PHP_METHOD(MongoDB_Driver_Cursor, getId) above, would Cursor::getId() ever return null? It looks like the return value would either be a CursorId or Int64.

The return type in Cursor.stub.php also wouldn't allow a null value (at least for PHP 8.0+).