Use return types and phpdoc #178

whikloj · 2023-07-20T19:32:42Z

This is to add some documentation (as I understand it) and types to arguments and functions.

This should have no impact on the code...except possibly for ACK/NACK.

It appears that Apollo sends a "ack" header on MESSAGE frames which is not part of the spec. That seems to have impacted the base Protocol class which was looking for this ack header in the Protocol::getAckFrame function.

Also but unrelated the in Protocol::getNackFrame a message-id header was being sent on all "nack" messages and was overwriting what may have been set previously.

Add utilities to composer

Alter Frame::getMessageId() to return either id or message-id as the frame doesn't know what version it is for

…behaviour

staabm

I like adding native types. It will require a new major version I guess, as adding parameter types can be a breaking change for existing code

whikloj · 2023-11-08T21:37:19Z

Hey @staabm,

I am good with making a new major release for this change. If you have time to review and merge this, we can cut a 6.0.0 release

staabm · 2023-11-09T13:39:01Z

I will have a closer look in the next view days

staabm · 2023-11-10T15:41:24Z

to make sure phpstan reports nullability issues we need either raise the phpstan level (which would also bring in a lot of other non-nullability related errors) or we enable nullability checking separately

see https://twitter.com/markusstaab/status/1710201565087256893

staabm · 2023-11-10T15:43:45Z

tests/Unit/Protocol/ProtocolTestCase.php

@@ -90,11 +90,11 @@ public function testAckVersionTwo()
        $instance = $this->getProtocol(Version::VERSION_1_2);

        $actual = $instance->getAckFrame(
-            new Frame(null, ['message-id' => 'id-value', 'ack' => 'ack-id-value']),


are these test-changes related to the type changes, or could be separated because unrelated?

staabm

~~github bug~~

staabm · 2023-11-10T15:45:49Z

src/Transport/Parser.php

@@ -149,17 +149,17 @@ public function setObserver(ConnectionObserver $observer)
     *
     * @return FrameFactory
     */
-    public function getFactory()
+    public function getFactory(): ?FrameFactory


the property is not-nullable

Suggested change

public function getFactory(): ?FrameFactory

public function getFactory(): FrameFactory

maybe we should add a native property type (maybe as a followup; and only for private properties)

staabm · 2023-11-10T15:48:27Z

src/Transport/FrameFactory.php

@@ -46,7 +46,7 @@ function ($command, array $headers, $body) {
     * @param boolean $legacyMode stomp 1.0 mode (headers)
     * @return Frame
     */
-    public function createFrame($command, array $headers, $body, $legacyMode)
+    public function createFrame($command, array $headers, $body, $legacyMode): Frame


adding return type in non-final classes is BC breaking for subclasses -> we really need a new major.

it might be useful to declare some classes final in case we don't want people to subclass them (followup PR)

staabm · 2023-11-10T15:49:36Z

src/Network/Connection.php

@@ -363,7 +363,7 @@ protected function getConnection()
     *
     * @return array


it would be great to add key/value types to array-return types (followup PR)

staabm · 2023-11-10T15:50:07Z

src/Network/Connection.php

@@ -199,7 +199,7 @@ public function __construct($brokerUri, $connectionTimeout = 1, array $context =
     *
     * @param callable|null $waitCallback


having callable signatures would also be awesome (followup PR)

staabm · 2023-11-10T15:50:43Z

src/Broker/RabbitMq/RabbitMq.php

+    public function getNackFrame(Frame $frame, ?string $transactionId = null, ?bool $requeue = null): Frame
    {
-        if ($this->getVersion() === Version::VERSION_1_0) {
-            throw new StompException('Stomp Version 1.0 has no support for NACK Frames.');
-        }
-        $nack = $this->createFrame('NACK');
+        $nack = parent::getNackFrame($frame, $transactionId);
        if ($requeue !== null) {
            $nack->addHeaders(['requeue' => $requeue ? 'true' : 'false']);
        }
-        $nack['transaction'] = $transactionId;
-        if ($this->hasVersion(Version::VERSION_1_2)) {
-            $nack['id'] = $frame->getMessageId();
-        } else {
-            $nack['message-id'] = $frame->getMessageId();
-            if ($this->hasVersion(Version::VERSION_1_1)) {
-                $nack['subscription'] = $frame['subscription'];
-            }
-        }
-
-        $nack['message-id'] = $frame->getMessageId();
        return $nack;


this looks like a change which should be done in a separate PR?
(without the type additions)

whikloj · 2023-11-10T20:55:18Z

What I discovered when starting this is that almost everything allowed null, because null gets passed around as defaults and in tests.

Adding types caused these nulls to cause errors so tests and codestyle and phpstan would not pass.

So I'm going to close this and consider removing all the uses of/acceptance of null first.

Then try types again. I will consider the other comments in the other PRs

whikloj added 11 commits July 18, 2023 15:25

Fix one test and add return types to Protocols

4f5af96

Add return types to Version

5bc082a

Add return types to Network Observers and Connection

3a91ba8

Add utilities to composer

Add return types to Broker supporting classes

93366fa

Add return types to States

f053d88

Add return type to Transport

08c3a67

Fix weird use of 'ack' headers

55fc568

Change subscription id to a string, as it defaults to the client id.

4f0fb0d

Alter Frame::getMessageId() to return either id or message-id as the frame doesn't know what version it is for

Apollo and OpenMq do not seem to support the Stomp spec for ACK/NACK …

87643a0

…behaviour

Add codesniffer to github actions

5f1e428

Add some more comments

6d286b9

staabm reviewed Jul 21, 2023

View reviewed changes

staabm reviewed Nov 10, 2023

View reviewed changes

whikloj closed this Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use return types and phpdoc #178

Use return types and phpdoc #178

whikloj commented Jul 20, 2023

staabm left a comment

whikloj commented Nov 8, 2023

staabm commented Nov 9, 2023

staabm commented Nov 10, 2023 •

edited

Loading

staabm Nov 10, 2023

staabm left a comment •

edited

Loading

staabm Nov 10, 2023

staabm Nov 10, 2023

staabm Nov 10, 2023

staabm Nov 10, 2023

staabm Nov 10, 2023

whikloj commented Nov 10, 2023

	public function getFactory(): ?FrameFactory
	public function getFactory(): FrameFactory

		@@ -363,7 +363,7 @@ protected function getConnection()
		*
		* @return array

		@@ -199,7 +199,7 @@ public function __construct($brokerUri, $connectionTimeout = 1, array $context =
		*
		* @param callable\|null $waitCallback

Use return types and phpdoc #178

Use return types and phpdoc #178

Conversation

whikloj commented Jul 20, 2023

staabm left a comment

Choose a reason for hiding this comment

whikloj commented Nov 8, 2023

staabm commented Nov 9, 2023

staabm commented Nov 10, 2023 • edited Loading

staabm Nov 10, 2023

Choose a reason for hiding this comment

staabm left a comment • edited Loading

Choose a reason for hiding this comment

staabm Nov 10, 2023

Choose a reason for hiding this comment

staabm Nov 10, 2023

Choose a reason for hiding this comment

staabm Nov 10, 2023

Choose a reason for hiding this comment

staabm Nov 10, 2023

Choose a reason for hiding this comment

staabm Nov 10, 2023

Choose a reason for hiding this comment

whikloj commented Nov 10, 2023

staabm commented Nov 10, 2023 •

edited

Loading

staabm left a comment •

edited

Loading