heroku / buildpacks-php

Heroku's Cloud Native Buildpack for PHP applications.
BSD 3-Clause "New" or "Revised" License
8 stars 2 forks source link

Composer json parsing doesn't account for scripts described as objects #81

Open cedricziel opened 7 months ago

cedricziel commented 7 months ago

Usually, composer scripts are arrays or sequences of commands. In symfony projects, a set of framework provided scripts is defined as objects:

{
    "scripts": {
        "auto-scripts": {
            "cache:clear": "symfony-cmd",
            "assets:install %PUBLIC_DIR%": "symfony-cmd",
            "importmap:install": "symfony-cmd"
        },
        "post-install-cmd": [
            "@auto-scripts"
        ],
        "post-update-cmd": [
            "@auto-scripts"
        ]
    }
}

This buildpack fails to build when a script is described as object.

Proposed solution

Allow scripts to be defined as objects as well

dzuelke commented 4 months ago

Hi @cedricziel!

Do you have further examples of this usage pattern? What exactly does the key in the object achieve? AFAICT it's simply ignored, Composer will only run the value part with the command string?

cedricziel commented 4 months ago

Hi @dzuelke

The CNB fails to parse the composer.json file entirely and can't complete the build. I think it's less an issue of with composer and more an issue with the way this build pack gathers metadata.

dzuelke commented 4 months ago

Yeah, I understand what's happening, @cedricziel.

My question was what this object notation is:

        "auto-scripts": {
            "cache:clear": "symfony-cmd",
            "assets:install %PUBLIC_DIR%": "symfony-cmd",
            "importmap:install": "symfony-cmd"
        },

Every value in the scripts object in composer.json should be either a string, or an array of strings, not an object. It works because Composer loads the JSON in array mode, so it can iterate over the values.

But Composer does not use the keys you provide there; this object syntax is not documented in the Composer docs, and the keys are not used inside Composer's EventDispatcher::doDispatch().

I researched, and it's a Symfony Flex thing, it magically expands these entries: https://github.com/symfony/flex/blob/4dc11919791f81d087a12db2ab4c7e044431ef6b/src/ScriptExecutor.php#L79-L91

I'll see what I can do to support this.

cedricziel commented 4 months ago

That would be great - I imagine this is a potential blocker for a large share of modern PHP apps and resolving it could help others adopt the CNB

dzuelke commented 4 months ago

That would be great - I imagine this is a potential blocker for a large share of modern PHP apps and resolving it could help others adopt the CNB

Yup for sure. Just not the easiest of things given how we're using serde to parse composer.json to Rust types, and Rust is quite particular about its types :)

cedricziel commented 4 months ago

I looked at it before I opened the ticket, but had just started with rust.

I think a struct for scripts with a custom serializer / deserializer could do that.

Essentially always having a Vec.