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

How to treat allow-plugin's warnings as errors? #10912

Closed
DmitryMaksimov opened this issue Jul 1, 2022 · 17 comments
Closed

How to treat allow-plugin's warnings as errors? #10912

DmitryMaksimov opened this issue Jul 1, 2022 · 17 comments
Labels
Milestone

Comments

@DmitryMaksimov
Copy link

My composer.json:

{
    "repositories": [
        {
            "type": "composer",
            "url": "https://wpackagist.org",
            "only": [
                "wpackagist-plugin/*",
                "wpackagist-theme/*"
            ]
        }
    ],
    "require": {
        "php": "7.4.*",
        "ext-xml": "*",
        "ext-curl": "*",
        "ext-mysqli": "*",
        "ext-mbstring": "*",
        "johnpbloch/wordpress": "5.9.3",
        "wpackagist-theme/twentytwentytwo": "1.2"
    },
    "extra": {
        "wordpress-install-dir": "pub/wp",
        "installer-paths": {
            "pub/wp-content/mu-plugins/{$name}/": [],
            "pub/wp-content/plugins/{$name}/": [
                "type:wordpress-plugin"
            ],
            "pub/wp-content/themes/{$name}": [
                "type:wordpress-theme"
            ]
        }
    },
    "config": {
        "allow-plugins": {
            "johnpbloch/wordpress-core-installer": true
        }
    }
}

When I run this command:

composer install

I get the following output:

No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.
Loading composer repositories with package information
Updating dependencies
Lock file operations: 8 installs, 0 updates, 0 removals
  - Locking composer/installers (v2.1.1)
  - Locking johnpbloch/wordpress (5.9.3)
  - Locking johnpbloch/wordpress-core (5.9.3)
  - Locking johnpbloch/wordpress-core-installer (2.0.0)
  - Locking wpackagist-theme/twentytwentytwo (1.2)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 8 installs, 0 updates, 0 removals
  - Installing johnpbloch/wordpress-core-installer (2.0.0): Extracting archive
  - Installing composer/installers (v2.1.1): Extracting archive
composer/installers contains a Composer plugin which is blocked by your allow-plugins config. You may add it to the list if you consider it safe. See https://getcomposer.org/allow-plugins
You can run "composer config --no-plugins allow-plugins.composer/installers [true|false]" to enable it (true) or keep it disabled and suppress this warning (false)
  - Installing johnpbloch/wordpress-core (5.9.3): Extracting archive
  - Installing johnpbloch/wordpress (5.9.3): Extracting archive
  - Installing wpackagist-theme/twentytwentytwo (1.2): Extracting archive
Generating autoload files
1 package you are using is looking for funding.
Use the `composer fund` command to find out more!

And I expected this to happen:

returned code different from 0, because we don't know allowed composer/installers or not

@stof
Copy link
Contributor

stof commented Jul 1, 2022

if you don't make a choice, the plugin will stay disabled (in interactive mode, it will ask instead). So ot me, there is no errors

@Seldaek
Copy link
Member

Seldaek commented Jul 1, 2022

Yeah I can see that there are cases where you may want to be alerted to this, but failing the process seems a bit extreme to me, and also it'd be really hard to implement cleanly I think. IMO this should anyway be caught at dev time when adding a dependency which is or requires a plugin.

@DmitryMaksimov
Copy link
Author

We use composer in deploy flow on remote machine. When composer deploys with some warnings we can't care about it, and our site isn't workable. So, may be some option to composer cli will help to us?

@Seldaek
Copy link
Member

Seldaek commented Jul 1, 2022

Well IMO you should really test these things first on local machine or staging at least, then you would discover the issue and could add the plugins. If you aren't able to do that it's probably safer (from a reliability perspective) to set composer config allow-plugins true, which will allow all plugins. It's not the safest from a security POV, but evil plugins finding their way into your dependencies is also a fairly low risk event, so this isn't a terrible thing to do IMO.

@Seldaek Seldaek added the Support label Jul 1, 2022
@DmitryMaksimov
Copy link
Author

from today composer config allow-plugins true isn't work

@Seldaek
Copy link
Member

Seldaek commented Jul 1, 2022

Yes it requires latest Composer as there was a bug, 2.2.15 and 2.3.8 releases fixed it earlier today.

@DmitryMaksimov
Copy link
Author

OK, thank you all.

@DmitryMaksimov
Copy link
Author

The new feature is interesting, but not configurable for remote machines. Sadly.

@DmitryMaksimov DmitryMaksimov closed this as not planned Won't fix, can't repro, duplicate, stale Jul 1, 2022
@DmitryMaksimov
Copy link
Author

May be you can add some event in scripts section? Something like "pre-package-block"?

@Seldaek Seldaek reopened this Jul 1, 2022
@Seldaek
Copy link
Member

Seldaek commented Jul 1, 2022

Yes that might be an option..

@Seldaek Seldaek added this to the 2.4 milestone Jul 1, 2022
@Seldaek Seldaek added Feature and removed Support labels Jul 1, 2022
@danepowell
Copy link
Contributor

This is really necessary in order to catch issues early in CI, otherwise they just go unnoticed.

Furthermore, as we face similar problems with uncaught warnings in other commands (#10241), I'd really prefer to have a global --treat-warnings-as-errors flag, or else a way to catch all warnings with composer validate, so we don't have to chase these problems down with an endless series of flags.

@damz
Copy link
Sponsor Contributor

damz commented Jul 4, 2022

We have received quite a number of support requests from our customers at Platform.sh because of this time-based backwards-compatibility break.

And yes, the key issue here is that composer install now just ignores plugins and still returns success. In many (possibly most) cases it just results into a completely broken application (because composer plugins are used to install modules in the right place for Drupal and Wordpress, etc.).

@Seldaek
Copy link
Member

Seldaek commented Jul 4, 2022

I think #10920 is a good solution as it fails hard when scripted/non interactive runs happen, thus avoiding any accidents when we cannot prompt the user.

Does that seem like a good outcome to everyone? I'll aim to release this tomorrow morning (UTC)

@marvinhinz
Copy link

We have received quite a number of support requests from our customers at Platform.sh because of this time-based backwards-compatibility break.

And yes, the key issue here is that composer install now just ignores plugins and still returns success. In many (possibly most) cases it just results into a completely broken application (because composer plugins are used to install modules in the right place for Drupal and Wordpress, etc.).

We were really scratching our heads too, some of our successful deployments started to act up strangely or were broken. Took us a bit to figure out that its due to missing allow-plugins config. We use a fixed composer version for all our deployment processes, and we didnt update the composer binary in that timeframe.

Im actually a bit in disbelief you guys added a time based bc break. I expect a versioned binary too keep its behaviour without external modification or updates. Yes i know now that there was a warning message, but many people apparently didnt notice too. At least an exception that would have stopped the deployments would have helped a lot.

@Seldaek
Copy link
Member

Seldaek commented Jul 5, 2022

Yes, as I wrote in another issue, we now realized we fucked up. We were focused on the update process at the time, and did not fully realize the consequences of the timebomb changing the aspirationally-stable install-from-lock behavior.

Anyway only thing we can do here is try to mitigate the damage, and I'm hoping #10920 will do that.

@Seldaek Seldaek closed this as completed in 92e1c26 Jul 5, 2022
@Seldaek
Copy link
Member

Seldaek commented Jul 5, 2022

@nicolas-grekas
Copy link
Contributor

It'd be great to allow a plugin to declare that it's fine to skip it in non interactive mode.

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

7 participants