Runtime errors thrown in aliases.php under PHP8+

[javiermarinros]

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]

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

[jinglundong]

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]

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]

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]

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

[jinglundong]

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

[jinglundong]

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

[javiermarinros]

It's okay, thank you for the help!

[javiermarinros]

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

[jinglundong]

Sorry that I dropped the ball. I'm proposed a fix as suggested at https://github.com/GoogleCloudPlatform/appengine-php-sdk/pull/95.

[javiermarinros]

Thank you! I also proposed a fix with several more fix for PHP 8 compatibility issues and some unused dependencies removals at #94

[jinglundong]

I approved and merged #94. I think the next step is to tag it as 2.1.1.

[jinglundong]

PHP 8.1 tests are failing after #94. Sending out a revert PR for review at https://github.com/GoogleCloudPlatform/appengine-php-sdk/pull/96.

[SimoTod]

We have the same issue, when using this dependency, we get a lot of noise in our logs. Do you have any plans for merging #95? Thanks

[jinglundong]

That's a fair question. I will context switch back to this and fix it. One challenge I/we have is that we need to keep this Github Repo and Google's internal code base in sync. I will investigate and discuss with my teammates.

[jinglundong]

Monty, let's talk about this offline when you have time.