Skip to content

Made patches.lock.json configurable #533#584

Open
MakerTim wants to merge 7 commits into
cweagans:mainfrom
MakerTim:main
Open

Made patches.lock.json configurable #533#584
MakerTim wants to merge 7 commits into
cweagans:mainfrom
MakerTim:main

Conversation

@MakerTim

@MakerTim MakerTim commented Jun 24, 2024

Copy link
Copy Markdown

Description

Relates to/closes #533

Related tasks

  • Documentation has been updated if applicable
  • Tests have been added
  • Does not break backwards compatibility OR a BC break has been discussed in the related issue(s).

Other notes

I'm new to the composer-plugin world, so have no clue that this is even the correct path.
Before writing docs and tests can someone confirm this is the correct path?

Comment thread src/Plugin/Patches.php Outdated
Comment on lines +155 to +157
if (!is_string($lock_file)) {
$lock_file = 'patches-lock-file';
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block isn't necessary anymore -- composer configurable plugin handles setting a default for you, so if it's not specified, it's whatever is hardcoded in the config above.

Comment thread src/Plugin/Patches.php
Comment on lines +159 to +163
if ($base === 'composer') {
return "$dir/$lock_file";
}

return "$dir/$base-$lock_file";

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, if you set COMPOSER=composer-foo.json and run any of the composer patches commands, you'll get composer-foo-patches.lock.json. If we set patches-lock-file to asdf.lock.json and also set COMPOSER=composer-foo.json, I think we'll get composer-foo-asdf.lock.json.

This isn't incorrect, but it is something that docs and tests will need to account for. Just flagging for visibility.

Comment thread src/Plugin/Patches.php
/**
* Get the path to the current patches lock file.
*/
public static function getPatchesLockFilePath(): string

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why this was a static function. Might be worth checking if this breaks any of the tests.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By not calling the method while registering, its fixed.
Parsing extra's just for the description is not worth it?

@MakerTim
MakerTim requested a review from cweagans June 10, 2025 07:03
@ceesgeene

Copy link
Copy Markdown

We use patches-file composer.patches.json and would love to be able to also use composer.patches.lock.json for consistency.

@cweagans
cweagans marked this pull request as draft October 30, 2025 23:43
@cweagans

Copy link
Copy Markdown
Owner

Converted this to a draft until docs and tests are added + review feedback is captured. Thanks for your work on this so far!

@MakerTim

Copy link
Copy Markdown
Author

Ha @cweagans

Did write the tests and docs
Anything else?

@MakerTim
MakerTim marked this pull request as ready for review April 28, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[2.*] Make lock file path configurable

3 participants