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

Fix #218: Respect composer lock #219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Oscaner
Copy link

@Oscaner Oscaner commented Jun 11, 2021

Respect composer lock, no update any packages during package install.

Copy link
Contributor

@mcaskill mcaskill left a comment

Choose a reason for hiding this comment

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

This condition was brought up during the refactor to support Composer 2 and bounced around a bit [1] [2].

Comment on lines +369 to +376
if ($this->state->forceUpdate()) {
// Force update mode so that new packages are processed rather
// than just telling the user that composer.json and
// composer.lock don't match.
$installer->setUpdate(true);
} else {
$this->logger->log('You may need to manually run composer update to apply merge settings');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent indentation:

Suggested change
if ($this->state->forceUpdate()) {
// Force update mode so that new packages are processed rather
// than just telling the user that composer.json and
// composer.lock don't match.
$installer->setUpdate(true);
} else {
$this->logger->log('You may need to manually run composer update to apply merge settings');
}
if ($this->state->forceUpdate()) {
// Force update mode so that new packages are processed rather
// than just telling the user that composer.json and
// composer.lock don't match.
$installer->setUpdate(true);
} else {
$this->logger->log('You may need to manually run composer update to apply merge settings');
}

Copy link
Member

Choose a reason for hiding this comment

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

This still needs fixing at least...

@raileanunalexandru
Copy link

raileanunalexandru commented Jun 16, 2022

We have tested this in quite a few scenarios and it's correctly respecting the intended functionality.
Can we have this released via a new tag so we can properly make use of it?
Patching this via composer doesn't work, so at the moment we have to reference this commit instead of the regular tag.

Maybe @reedy if you could have a look please?

Comment on lines +369 to +376
if ($this->state->forceUpdate()) {
// Force update mode so that new packages are processed rather
// than just telling the user that composer.json and
// composer.lock don't match.
$installer->setUpdate(true);
} else {
$this->logger->log('You may need to manually run composer update to apply merge settings');
}
Copy link
Member

Choose a reason for hiding this comment

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

This still needs fixing at least...

@raileanunalexandru
Copy link

Hi @reedy thank you for your activity on this.
Since this seems to have been forgotten by the initiator, i have made a new pull request to address this by fixing the indentation giving credit to the author.
Could we maybe close this pull request the have a look into the new one here?
#232

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants