feat(cluster): add a ProtocolVersion enum for withProtocolVersion() - #153
Open
CodeLieutenant wants to merge 1 commit into
Open
feat(cluster): add a ProtocolVersion enum for withProtocolVersion()#153CodeLieutenant wants to merge 1 commit into
CodeLieutenant wants to merge 1 commit into
Conversation
`withProtocolVersion()` took a bare int, so a caller had to know that the
native protocol version is the byte the protocol itself defines, and a typo
produced a runtime error rather than a type error.
Add `Cassandra\ProtocolVersion`, an int-backed enum whose cases carry that
byte, so V1 through V5 match CASS_PROTOCOL_VERSION_V1 through _V5:
->withProtocolVersion(Cassandra\ProtocolVersion::V4)
The method now accepts the enum or an int. The int path stays because the
enum names only the five native versions, and the driver also understands
versions it does not name, such as the DSE ones. It keeps its own range check.
`Cluster\Builder`'s `protocolVersion` property now reports the matching enum
case when the value maps to one, and falls back to the plain int when it does
not. Code that read that property as an int and mapped a named version will
see an enum instead.
The descriptor generator did not understand enums: it emitted a create_object
handler and an object-allocating registration for every class it saw. Teach it
the enum kind, so an internal enum registers through
zend_register_internal_enum() with no instance handlers, since its cases are
immutable objects the engine builds.
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
withProtocolVersion()took a bare int, so a caller had to know that the native protocol version is the byte the protocol itself defines, and a typo produced a runtime error instead of a type error.Adds
Cassandra\ProtocolVersion, an int-backed enum whose cases carry that byte, soV1throughV5matchCASS_PROTOCOL_VERSION_V1through_V5.Why the int overload stays
The enum names only the five native protocol versions, but the driver also understands versions it does not name, such as the DSE ones. So the signature is
ProtocolVersion|intand the int path keeps its own range check.ProtocolVersion::V43ProtocolVersion::V3660InvalidArgumentExceptionBehaviour change worth noting
Cluster\Builder'sprotocolVersionproperty now reports the matching enum case when the value maps to one, and falls back to the plain int when it does not. Code that read that property as an int and used a named version will now see an enum. Reading->valuecovers both.Descriptor generator
The generator did not understand enums: it emitted a
create_objecthandler and an object-allocating registration for every class it saw, which is wrong for an internal enum whose cases the engine builds and owns. It now recognises the enum kind and registers throughzend_register_internal_enum()with no instance handlers.This is the part most worth a careful read, since it changes code generation for every stub, not just this one.
Verification
Cluster/Builder.c.Note on the branch
This branch previously carried #152, which is merged. Its three commits were dropped during a rebase onto
trunkas already upstream, so this PR is the single new commit. The branch name is left over from that work and does not describe this change.