added version method to AProtocol classes

[transistive]

This is a quality of life suggestion.

In the newer versions, the client needs to use instanceof methods to detect versions. It is a lot easier to request the version of any protocol with a simple version method. This simplifies code like this to a one liner:

public static function determineBoltVersion(V3 $bolt): string
{
        if ($bolt instanceof V4_4) {
            return '4.4';
        }
        if ($bolt instanceof V4_3) {
            return '4.3';
        }
        if ($bolt instanceof V4_2) {
            return '4.2';
        }
        if ($bolt instanceof V4_1) {
            return '4.1';
        }
        if ($bolt instanceof V4) {
            return '4.0';
        }

         return '3';
}

Gets simplified to:

public static function determineBoltVersion(V3 $bolt): string
{
         return $bolt->getVersion();
}

The version notation is the same as these (found on https://7687.org/):

What do you think @stefanak-michal?

[stefanak-michal]

you still can get protocol version on Bolt instance https://github.com/neo4j-php/Bolt/blob/release_v3/src/Bolt.php#L119

isn't that enough?

[transistive]

In theory it is, but it requires me to keep track of bolt factory object, as well as the protocol object, which is a bit of an antipattern.

[stefanak-michal]

then what about something simple?

//class AProtocol

/**
 * Get protocol class version
 * @return string
 * @throws Exception
 */
public function getVersion(): ?string
{
    if (preg_match("/V([\d_]+)$/", static::class, $match))
        return str_replace('_', '.', $match[1]);
    throw new Exception('Protocol version class name is not valid');
}

[stefanak-michal]

also if you will get version through protocol itself the method in Bolt getProtocolVersion() shouldn't be deprecated?

[transistive]

Depends on what you call simple :sweat_smile:

It would reduce code duplication at the cost of introducing the performance hit of using regex. I'm ok with the tradeoff if you are.

Regarding the Bolt::getProtocolVersion: I feel it should be removed or deprecated, yes. It should only able to return the versions it used to negotiate the protocol, not the latest one it created

[stefanak-michal]

you can replace regex with some string methods.. explode at slash, take last part, remove V and replace _ with .

but what is fast? and it's really that big hit on performance? this one regex?

[transistive]

Yes, it is negligible, to me, it is more about decoupling the name of the class from the actual protocol, but both work!

[transistive]

Can I rename V4 to V4_0 to keep in line with the version scheme found at 7867.org?

[stefanak-michal]

Can I rename V4 to V4_0 to keep in line with the version scheme found at 7867.org?

I tried to connect with version 4 and check what returns neo4j over bolt.. it's 4

Which means I should keep it, because the protocol itself is above documentation

[transistive]

Agreed! I will keep it like it is then. I changed the method to your implementation

[stefanak-michal]

Also I tried performance test on regex against the logic I've described and regex is faster.

[stefanak-michal]

you should also remove method in Bolt

[stefanak-michal]

I changed float to string for version in Bolt, because I realized looking at code it's stupid. Because there can be version 4.4.1 for example. And I had to do small abbrevation about cleaning up version array, because the version can be 4.0.1. Which means I can't just call array_filter.

[transistive]

Looks like that's it!

Thanks :+1: