Problem/Motivation

Tweak package_manager/src/ProcessOutputCallback.php in a test context, because in a test context we may want Composer deprecations to cause test failures, so we are notified of upstream changes and can update our implementation to track upstream.

https://git.drupalcode.org/project/automatic_updates/-/merge_requests/71...

Steps to reproduce

N/A

Proposed resolution

Bring back what @omkar.podey's earlier MR716 for #3337667: Allow JsonProcessOutputCallback and other Composer runner callbacks to gracefully handle deprecated command options did previously:

    if ($error !== '' && !str_contains($error, 'deprecated') && !str_contains($error, 'deprecation')) {
      throw new \Exception($error);
    }

… but only in a test context.

We already have prior art for detecting whether we're inside a test or not: \Drupal\package_manager\Validator\PhpExtensionsValidator::insideTest() (which is private static and can be copied in its entirety).

Remaining tasks

  1. Write a test that simulates composer showing a deprecation error in its output; this test should expect an exception.
  2. Make the test pass by ensuring that this exception is thrown when appropriate.

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork automatic_updates-3349004

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

omkar.podey created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Let composer deprecations to cause test failures » [PP-1] Let Composer deprecations to cause test failures
Status: Active » Postponed
Issue tags: +maintainability, +Needs tests

Blocked on #3337667: Allow JsonProcessOutputCallback and other Composer runner callbacks to gracefully handle deprecated command options.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Let Composer deprecations to cause test failures » [PP-1] Add tests that fail on Composer deprecations
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Title: [PP-1] Add tests that fail on Composer deprecations » Add tests that fail on Composer deprecations
Status: Postponed » Active
Issue tags: +core-mvp, +beta target, +Needs issue summary update

Needs proposed solution

omkar.podey’s picture

omkar.podey commented

Maybe since ProcessOutputCallback::getOutput is logging warnings using 

$this->logger->warning($error_output);

we can use the count for warnings to be sure that there aren't any new deprecations. But the logger is getting reset at multiple points so this might not be feasible.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » omkar.podey
Issue summary: View changes
Issue tags: -Needs issue summary update

Discussed with @omkar.podey earlier today.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Add tests that fail on Composer deprecations » Update ProcessOutputCallback to fail on Composer deprecations, to get early notifications of Composer changes

omkar.podey opened merge request !887

omkar.podey’s picture

omkar.podey commented
Assigned: omkar.podey » Unassigned
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +sprint

LGTM! 👍😄 Thanks!

omkar.podey’s picture

omkar.podey commented
Status: Reviewed & tested by the community » Needs review

Wim Leers credited phenaproxima.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Why has this not been committed in >1 month time? 😬😳

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs work

Why not indeed!

A few very small things, but otherwise this looks good.

omkar.podey’s picture

omkar.podey commented
Assigned: Unassigned » omkar.podey
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: omkar.podey » tedbow
Status: Needs work » Needs review

Decided to make the test coverage a tad more thorough; assigning to @tedbow for final review.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: tedbow » phenaproxima
Status: Needs review » Needs work
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

Another way to solve this problem would be to ask composer( and maybe provide a PR) to add a --no-deprectations flag. Basically have composer fail if any deprecations are used. I am not sure how else you could write tests that rely on composer commands and be alerted to deprecations

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: phenaproxima » Unassigned
Status: Needs work » Postponed

Hey everyone. Thanks for all the work on this!

But I think we need to rethink why we need this issue at all.

Presumably we need this issue because we don't want to be surprised when Composer 3 out comes and have our code break because we are using deprecated options or commands that will throw exceptions in Composer 3. The other reason would be to be helpful to other developers that might be using the Package Manager API and using options or commands that we don't use and that also might be deprecated.

The best way to solve this would be to use a matrix of composer versions in our tests. Right now there 2 problems with this.

  1. There are no Composer 3 releases we could test against https://github.com/composer/composer/releases
  2. Drupalci doesn't give us the ability to use matrices
  3. We don't want to switch to Gitlab Actions now because although it is available to use on all contrib projects Drupal core does not use it and any changes we made to our testing we would have to remove when we tried to get into core

Because there are not any Composer 3 pre-releases and \Drupal\package_manager\ComposerInspector::SUPPORTED_VERSION restricts Package Manager from using Composer 3 even it is was available there is zero chance right now anyone would be using Package Manager with Composer 3 and it will probably be a while.

Since we are blocked on getting into Drupal core because of needed changes to Drupal.org infrastructure and related security reviews it is likely that by the time we get into Drupal core it will already be using gitlab actions and then we would be free to use it ourselves to run our tests on a matrix of Composer versions. We would probably want to do that anyways to avoid problem unrelated to declared deprecations such as we found in #3351594: As of Composer 2.5.5, `composer config` JSON-encodes boolean values

Even if core is not using Gitlab actions by the time we get into core we will still be an experimental module so I think it would be fine not to have warnings for Composer deprecations at that point.

I am postponing this issue for now and we can reopen when we get closer to getting into core if are not able to switch to Gitlab actions at that point