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

Made patches.lock.json configurable #533 #584

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MakerTim
Copy link

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 on lines 155 to 157
if (!is_string($lock_file)) {
$lock_file = 'patches-lock-file';
}
Copy link
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 on lines +159 to +163
if ($base === 'composer') {
return "$dir/$lock_file";
}

return "$dir/$base-$lock_file";
Copy link
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.

/**
* Get the path to the current patches lock file.
*/
public static function getPatchesLockFilePath(): string
Copy link
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
Owner

Choose a reason for hiding this comment

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

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

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
2 participants