Runtime errors thrown in aliases.php under PHP8+

javiermarinros commented 2 years ago

A runtime error is thrown for most of class alias defined in src/aliases.php of this kind:

Cannot declare class google\appengine\api\modules\InvalidModuleStateException, because the name is already in use

Given that PHP namespaces are case-insensitive, most of the alias defined there are not necessary, as the old php55 sdk names are case-insensitive equal to the php7+ SDK.

javiermarinros commented 2 years ago

@zachluna @pmaloogoogle would you take a look at this, please?

jinglundong commented 2 years ago

Thanks for reporting this issue, @javiermarinros!

Do you mind helping us understand this issue further? aliases.php was used by the autoload. Does it mean the whole autoload functionality is broken? Are theses aliases unnecessary or blocking the common usage of this SDK?

Seems like we are missing test coverage on the use cases that could trigger this error. Do you have any recommendation about reproducing this issue, so we could add tests?

javiermarinros commented 2 years ago

Hi @jinglundong, thank you for your help

You can replicate this error deploying a test php project into AppEngine with this setup:

app.yaml

runtime: php81
app_engine_apis: true

entrypoint: serve app.php

handlers:
  - url: .*
    script: auto

app.php

<?php

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(-1);

set_error_handler(
    static function (int $severity, string $message, string $file, int $line, mixed $context = null): bool {
        echo '<pre>'.print_r(func_get_args(), true).'</pre>';
        return true;
    },
    error_reporting()
);
set_exception_handler(
    static function (Throwable $exception) {
      echo '<pre>'.print_r($exception, true).'</pre>';
    }
);

require 'vendor/autoload.php';

echo "Load done\n";

The issue can be resolved by two approaches:

Remove unnecessary aliases, as most of them try to alias a class that is case-insensitive equal to the original one, and PHP can handle namespace and class names in a case-insensitive way. This would have better performance as it will reduce unnecessary class_alias invocations. This would leave alias.php like this:

$classMap = [
    'Google\AppEngine\Api\AppIdentity\AppIdentityService' => 'google\appengine\api\app_identity\AppIdentityService',
];

foreach ($classMap as $class => $alias) {
    @class_alias($class, $alias);
}

The other method would be checking if the alias class exists before aliasing, but it doesn't make much sense to try to alias already existing classes in every request:
```
foreach ($classMap as $class => $alias) {
if (!class_exists($alias, false)) {
    class_alias($class, $alias);
}
}
```

I hope this info can help you.

jinglundong commented 2 years ago

Thanks for the detailed explanation and suggestions! I agree with your analysis and would prefer removing the unnecessary aliases as well. I don't see any down sides of this approach.

From my reading of https://www.php.net/manual/en/language.namespaces.rationale.php, I believe namespaces are case-insensitive for all the php versions (that GAE supports). So, we don't need to worry about causing problems in specific php versions.

Sorry for the late response, since I've been out of the office in the past two weeks. I will try to run your sample and probably send out a PR next week.

javiermarinros commented 2 years ago

Hi @jinglundong have you had time to prepare the PR? Thank you.

jinglundong commented 2 years ago

Sorry, I totally dropped the ball. I will give it another shot this week.

jinglundong commented 2 years ago

I started the fix, but got delayed by a high severity CVE. Will continue the work next week. Sorry for all the delays.

javiermarinros commented 2 years ago

It's okay, thank you for the help!

javiermarinros commented 1 year ago

Hi @jinglundong do you have any news about this fix?