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

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

Open
cedricziel opened this issue Feb 9, 2024 · 6 comments · May be fixed by #132
Open

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

cedricziel opened this issue Feb 9, 2024 · 6 comments · May be fixed by #132
Assignees
Labels
question Further information is requested

Comments

@cedricziel
Copy link

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 dzuelke self-assigned this Jun 3, 2024
@dzuelke dzuelke added the question Further information is requested label Jun 3, 2024
@dzuelke
Copy link
Contributor

dzuelke commented Jun 3, 2024

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
Copy link
Author

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
Copy link
Contributor

dzuelke commented Jun 5, 2024

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
Copy link
Author

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
Copy link
Contributor

dzuelke commented Jun 5, 2024

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
Copy link
Author

cedricziel commented Jun 5, 2024

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.

cedricziel added a commit to cedricziel/buildpacks-php that referenced this issue Sep 7, 2024
cedricziel added a commit to cedricziel/buildpacks-php that referenced this issue Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants