feat: introduce declarative function signatures

[anniefu]

Allow expressing the function signature type configuration through code
instead of through FUNCTION_SIGNATURE_TYPE env var. This means
the FUNCTION_SIGNATURE_TYPE var can be omitted when deploying
a function with a declarative function signature.

The new signature also changes to using the official CloudEventInterface from the CloudEvents SDK instead of the Function Framework's custom class.

The primary difference is $cloudevent->getTime() returns a DateTime instead of a string. This can be converted back to a string as follows:

$cloudevent->getTime()->format(DateTimeInterface::RFC3339_EXTENDED);

Define your Function

index.php

<?php

require 'vendor/autoload.php';

use Psr\Http\Message\ServerRequestInterface;
use Google\CloudFunctions\FunctionsFramework;
use CloudEvents\V1\CloudEventInterface;
 
FunctionsFramework::http("helloHttp", func(ServerRequestInterface $request)
{
   return "Hello World from a PHP HTTP function!" . PHP_EOL;
});
 
FunctionsFramework::cloudEvent("helloCloudEvents", func(CloudEventInterface $cloudevent)
{
   // Print the whole CloudEvent
   $stdout = fopen('php://stdout', 'wb');
   fwrite($stdout, $cloudevent);
});

Run your function

php -S localhost:8080 vendor/bin/router.php

[matthewrobertson]

Do we not need to pass in the $signatureType to preserve backwards compatibility here? ie GCFv1 customers today might be using non-declarative cloudevent functions in PHP

[anniefu]

It still respects the env var, I just moved it from being read here to being read within invoker.php so I could test it properly. router.php is hard to test because it's being used as script instead of a just declaring a function or class.

[bshaffer]

There are a few reasons why this is not a good idea

1. This breaks backwards compatibility
2. The decision to inject the environment variable was intentional. This is a best practice (see dependency injection), as it keeps the logic of where the value comes from (e.g. the environment) separate from the class which uses the value (e.g. the Invoker)
3. Having the value be injectable is more testable, as you can test the class without having to mess with environment variables.

[bshaffer]

Please do not tag a release until we resolve the backwards compatibility breaking issues

[bshaffer]

There are a few reasons why this is not a good idea

1. This breaks backwards compatibility
2. The decision to inject the environment variable was intentional. This is a best practice (see dependency injection), as it keeps the logic of where the value comes from (e.g. the environment) separate from the class which uses the value (e.g. the Invoker)
3. Having the value be injectable is more testable, as you can test the class without having to mess with environment variables.

[bshaffer]

This is not optimal because you're changing the constructor of FunctionsWrapper, which has a $signature parameter that is now unavailable.

This is breaking backwards compatibility and also making that parameter unavailable to users.

[anniefu]

I can add back the $signature optional parameter.

[bshaffer]

It was an intentional choice to have the environment variable passed in. Injecting the environment variables is a better practice, easier to test, more reliable than having the logic exist in the Invoker itself. Also, this breaks backwards compatibility, if users were calling the Invoker directly. I think we need to roll back this change.

[anniefu]

The issue I ran into was that I couldn't test CloudEvent functions properly with this code in router.php, because it is just a script and I couldn't figure out how to pass it a CloudEvent request.

I can add back the signature type parameter here, and then only read the env var if it's missing. That way it is not a change to the constructor?

Can we mark these classes as internal? In general, I don't think we want people to be using these directly.

[bshaffer]

For more idiomatic PHP, this should be removed

[anniefu]

Sounds good!

[bshaffer]

In general static methods are something we try to avoid. It would be better to make these methods instance methods, and instantiate the registry at runtime.

[anniefu]

Ah ok, we can use a singleton pattern instead.