fix(PubSub): publisher compression type

[vishwarajanand]

b/289990087

[bshaffer]

This is a bit worrisome because, while removing the typehint might get rid of the error, it's not the best way to fix this issue. Instead we should be looking at why these properties weren't initialized by the time they were accessed. They're being set properly in the constructor, so they should be initialized in every instance they're used.

My guess is that since we're using Sysv, there may be some strange behavior here. Those values aren't showing up on the other side when passed to the batch daemon.

When I switched the order so that the call to setCommonBatchProperties happened after setting those properties, the error went away. I believe this is because when this method is called, it looks at what the initialized properties are, and only serializes those. So we need to initialize them beforehand.

I think this is a better solution, so please test and see if it works!

google-cloud-php/PubSub/src/BatchPublisher.php

Lines 90 to 96 in 5bf356c

	$this->setCommonBatchProperties($options + [
	'identifier' => sprintf(self::ID_TEMPLATE, $topicName),
	'batchMethod' => 'publishDeferred'
	]);
	$this->enableCompression = $options['enableCompression'] ?? false;
	$this->compressionBytesThreshold = $options['compressionBytesThreshold'] ??
	Topic::DEFAULT_COMPRESSION_BYTES_THRESHOLD;

(move lines 90-93 to be after line 96)

[yash30201]

This is a bit worrisome because, while removing the typehint might get rid of the error, it's not the best way to fix this issue. Instead we should be looking at why these properties weren't initialized by the time they were accessed. They're being set properly in the constructor, so they should be initialized in every instance they're used.

My guess is that since we're using Sysv, there may be some strange behavior here. Those values aren't showing up on the other side when passed to the batch daemon.

When I switched the order so that the call to setCommonBatchProperties happened after setting those properties, the error went away. I believe this is because when this method is called, it looks at what the initialized properties are, and only serializes those. So we need to initialize them beforehand.

I think this is a better solution, so please test and see if it works!

google-cloud-php/PubSub/src/BatchPublisher.php

Lines 90 to 96 in 5bf356c

	$this->setCommonBatchProperties($options + [
	'identifier' => sprintf(self::ID_TEMPLATE, $topicName),
	'batchMethod' => 'publishDeferred'
	]);
	$this->enableCompression = $options['enableCompression'] ?? false;
	$this->compressionBytesThreshold = $options['compressionBytesThreshold'] ??
	Topic::DEFAULT_COMPRESSION_BYTES_THRESHOLD;

(move lines 90-93 to be after line 96)

Thank you so much @bshaffer for zeroing this in🔥.
I was investigating why this was happening and cornered down to Sysv but wasn't able to understand the exact issue. This was a valuable insight.🙌

I've raised a PR fixing this. #6423

[yash30201]

Discussed internally offline that since the case when we do not use type checks eliminates the issue, hence we should go ahead with priortising the fixes until we properly figure out what exactly is causing this issue.

[vishwarajanand]

Thanks for looking into this @bshaffer. We fixed your recommendation.
But think we should merge this as well, for two reasons:

1. Seems some linters still complain (maybe they are checking on older PHP versions?) for private int $var_name; and private bool $var_name; while our lint has no issue with it.
2. These are the only two source files to follow the explicit type declaration pattern.

I am also not sure why not setting the properties in setCommonBatchProperties could lead to a variable not initialized error, when it's always initialized to false by default in constructor.

[bshaffer]

My main concern with the approach here is that it will fix the fatal error (which is obviously super important) but won't fix the underlying issue of the expected variable values being passed to the batch runner. I think if you test, you'll see that even without the typehints, those values are not initialized as expected.

I don't mind getting rid of the typehint - they aren't that important. But we still need to switch the order if we want the local variables to be set.

EDIT: I see now we already merged the other PR, so this comment is not necessary!

As for the WHY this works - take this example:

class ExampleFoo
{
    protected string $typedVariable1;
    protected string $typedVariable2;
    protected $untypedVariable;

    public function __construct() 
    {
        // initialize one of the typed variables, but not the other
        $this->typedVariable1 = 'initialized!';
    }

    public function getObjectVars()
    {
        return get_object_vars($this);
    }
}

In this example, we initialize one typed variable, but not the other. When we call getObjectVars on this object, only the typed variable we initialized, along with the untyped variable, will show up:

$f = new ExampleFoo();
var_dump($f->getObjectVars());
// array(2) {
//  ["typedVariable1"]=>
//  string(12) "initialized!"
//  ["untypedVariable"]=>
//  NULL
//}

This is a guess, I'd need to do more digging to be sure.

I no longer require changes

[bshaffer]

Seems some linters still complain

Can you give me an example of this?

These are the only two source files to follow the explicit type declaration pattern

I found that strange as well - I wonder when they were added?

I removed my review so feel free to merge it if you feel it's important!

[vishwarajanand]

It seems it was complaining because of an older php version. When I test the following code in php7.3 or below it complains.

Checked the below code in http://phptester.net/

<?php

class MyClass {
    private int $myInt;
    private bool $myBool;
}