ThePHPF / pie-design

97 stars 1 forks source link

Questions over Composer package naming #8

Closed asgrim closed 6 months ago

asgrim commented 6 months ago

To record a discussion so far on Composer package naming.

Current plan: the Composer package name is something like ext-debug in composer.json. However, as per composer/packagist#1440, this would need specific support to be added to Packagist.

How are names decided currently?

as far as I understand it, the ext author decides the name, but then it has to be approved and added to pecl.php.net by hand; that would be done by requesting on the pecl mailing list, so if there was an objectionable name for some reason, it would be discussed prior

What about using vendor/ prefixes?

An advantage of using vendor/package format, is it allows multiple implementations; for example if I wanted to fork xdebug, and install my custom xdebug, I could do pie install asgrim/xdebug instead of pie install xdebug/xdebug to get my own fork.

This is an option, but we would still need a way to either derive or determine the extension name. Some options

Questions - retroactive vendor prefixes

If we do add vendor/ prefix, how will we decide vendor prefix for existing extensions? For some, it is easy where a vendor is already used by the author(s), e.g. ext-xdebug could use xdebug/xdebug, ext-datadog-trace could use datadog/datadog-trace, etc.; however, some it might be unclear; e.g. ssh2. One could assume it should become php/ssh2 but there are implications to this (since at this point, it could conceivably be that php/ is a special vendor, and may even already be reserved by Composer/Packagist, for instance (would need to check this).

General points raised

Seldaek commented 6 months ago

The php vendor is indeed reserved, but I'm happy to let the php project use it if needed for php project packages of course.

asgrim commented 6 months ago

I'm personally thinking that a reasonable approach for this is:

This gives us the flexibility of having vendor namespaces, as well as reducing the attack vector @naderman has raised

alcaeus commented 6 months ago

I think using vendor fixes are preferable to using the old ext-<name> package format, especially as it avoids special logic to tie the ext- prefix to a specific type in composer.json. The packagist versions of extensions can then use provide to satisfy the composer dependencies of other libraries on an ext-<name> package. All in all, the proposal sounds good to me.

stof commented 6 months ago

Using a different name for the registered PIE extension than for the platform package used by composer for requirements in PHP packages actually introduces a new attack vector for typosquatting: packagist would not have any way to decide what is the source of truth for ext-xdebug as many php-ext packages could be providing that name or be named */xdebug (depending on the selected solution).

Note that the composer way of using a fork is not to register that fork on packagist under a new name (renaming a composer package while satisfying existing requirements is possible, but it is not a simple task, especially if you want it to be retroactive). The way to achieve this in composer is to register additional package repositories instead, with higher priority.

And changing composer so that it deals with namespaced names for platform packages would be a huge BC break for the composer ecosystem (and probably not that easy to implement in Composer as a bunch of places are currently relying on the fact that platform packages are not namespaced to identify them as they require special handling anyway due to not being installed per project by composer).

Seldaek commented 6 months ago

@stof the extensions are installed by pie, and not by composer. Composer will see them as regular ext-<name> packages in the PlatformRepository.

And on the pie side namespacing reduces the potential for typo squatting as it does for the other packages. Because you can't register ext-xdepug next to ext-xdebug, you'd have to register xdepug/xdebug which well is typo prone sure but a bit less, and anyway we have systems in place to catch this.

Note that the composer way of using a fork is not to register that fork on packagist under a new name (renaming a composer package while satisfying existing requirements is possible, but it is not a simple task, especially if you want it to be retroactive). The way to achieve this in composer is to register additional package repositories instead, with higher priority.

As for that, I disagree. If you're forking for the long term maintenance then yes submitting another package name is the way to do this, and you can't use replace/provide there because it won't be resolved by composer, so the extension name has to match the original.

If you're forking for a temporary bugfix or whatever, then yeah what you said is more accurate, but these aren't forks per se, and I'm not sure how these will be best handled by pie but I suspect perhaps it's just best left to whoever patches to figure it out how to install their patched extension. This is a special case and way less common than php userland patching I'm thinking.

asgrim commented 6 months ago

Thank you everyone for your feedback, it is greatly appreciated. I don't think there is a perfect solution to this; but given the way people are used to using Composer, I propose a combination of all 3 originally suggested options; with the known drawback that there is a slight disconnect between the Composer package name, and the "extension name":

a complete example:

{
    "name": "asgrim/example-pie-extension",
    "type": "php-ext",
    "license": "MIT",
    "description": "Example PIE extension",
    "require": {
        "php": ">=7.1,<8.4"
    },
    "provide": {
        "ext-example-pie-extension": "*"
    },
    "php-ext": {
        "extension-name": "ext-example-pie-extension",
        "priority": 74,
        "configure-options": []
    }
}

The only thing is; I am not sure what (if any) benefits we have from specifying provide, but it may have some semantic benefit later down the line, hence my suggestion to keep it optional for now.

If there are no further objections to this, I will update the design documentation accordingly and produce a PR this morning.

Seldaek commented 6 months ago

Yeah I am not sure either about the provide, it's probably harmless/useless. Otherwise sounds good to me 👍🏻

naderman commented 6 months ago

The semantics of provide in Composer however are that you can install multiple packages providing the same thing in parallel. More accurate would be replace, which implies a conflict and would better model the reality that two extensions with the same name cannot coexist?

asgrim commented 6 months ago

The semantics of provide in Composer however are that you can install multiple packages providing the same thing in parallel. More accurate would be replace, which implies a conflict and would better model the reality that two extensions with the same name cannot coexist?

Thank you for correcting me there; will amend the PR now!