Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scope "ext" classes if they're installed with composer too #1109

Open
kkmuffme opened this issue Jan 23, 2025 · 3 comments
Open

Scope "ext" classes if they're installed with composer too #1109

kkmuffme opened this issue Jan 23, 2025 · 3 comments
Labels

Comments

@kkmuffme
Copy link

Bug report

Question Answer
PHP-Scoper version 0.18.14
PHP version 8.3.0
{
    "name": "foo",
    "type": "library",
    "homepage": "foo.com",
    "authors": [
        {
            "name": "foo"
        }
    ],
    "minimum-stability": "stable",
    "require": {
        "php": "~8.1.0 || ~8.2.0 || ~8.3.0",
        "googleads/google-ads-php": ">=22"
    },
    "config": {
        "optimize-autoloader": true,
        "classmap-authoritative": true,
        "platform": {
            "php": "8.1.0",
            "ext-protobuf": false,
            "ext-grpc": false
        },
        "platform-check": true
    }
}

default scoper config

On my machine where I run scoper, I have grpc (and protobuf) installed for PHP - since I set it to false for the platform, composer (correctly) installed both packages.

However scoper somehow created a weird mix of exposed and scoped that will lead to fatals.
e.g. vendor\grpc\grpc\src\lib\BaseStub.php has a scoped namespace (correct): namespace Foo\Grpc;
It also correctly scoped the code in it, e.g.

if (!\method_exists('Foo\\Grpc\\ChannelCredentials', 'isDefaultRootsPemSet') || ...

However in vendor\google\gax\src\Transport\GrpcTransport.php all use statements are correctly scoped, except:

use Grpc\Channel;
use Grpc\ChannelCredentials;

I guess this happens bc scoper sees that these classes are available in PHP natively and treats them as exposed (just like use Exception; which is obviously correct)

Possible solutions:

  1. check composer platform for ext and if false, don't automatically expose
  2. if the class exists in composer packages to be scoped too, then also scope it, even if it exists natively on the system

I guess 2 is easier?

@theofidry
Copy link
Member

However in vendor\google\gax\src\Transport\GrpcTransport.php all use statements are correctly scoped, except:

use Grpc\Channel;
use Grpc\ChannelCredentials;

I guess this happens bc scoper sees that these classes are available in PHP natively and treats them as exposed (just like use Exception; which is obviously correct)

Correct, if a symbol is internal (i.e. comes from PHP or an extension), it should not be scoped.

There is however an exception to the above, and that is when you are dealing stubs. The way PHP-Scoper handles it is keep scoping as usual, but add an alias in the scoper-autoload.php.

if (!\method_exists('Foo\\Grpc\\ChannelCredentials', 'isDefaultRootsPemSet') || ...

If Grpc\ChannelCredentials is an internal symbol (you can check that with php-scoper inspect-symbol' 'Grpc\ChannelCredentials')

If you can provide a reproducer that I could check out it would be highly appreciated. I can't promise to be able to take a look soon, but that would make it a lot easier to find what is wrong. The method_exists() here is a bit suspicious too me because it's using a prefixed version which I don't think should be the case.

@theofidry theofidry added the bug label Jan 24, 2025
@kkmuffme
Copy link
Author

The issue arises bc grpc and protobuf have a native PHP extension (in C) but the same classes are also available as composer packages.
This isn't a stub.

If you can provide a reproducer

Use the composer.json provided in the original post and look at vendor\grpc\grpc\src\lib\BaseStub.php - all correct

Now install grpc + protobuf PHP extensions
Delete vendor
Run again and look at that file again.

@theofidry
Copy link
Member

@kkmuffme the problem with just looking at the file is that it is hard to ensure it is enough. PHP-Scoper doesn't operate on the file alone, but also via aliases it declares in the autoload. So one may check it and be under the impression it is not working because it gets prefixed, but it may be expected because you get the alias in the autoload. So whilst I can do the above, it's not guaranteed Ill get it right just from that

but the same classes are also available as composer packages.
This isn't a stub.

Apologies I meant a polyfill there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants