Skip to content

Fix #218: Respect composer lock - #219

Open
Oscaner wants to merge 1 commit into
wikimedia:masterfrom
ciandt-apac-dev:2.x-respect_composer_lock
Open

Oscaner wants to merge 1 commit into
wikimedia:masterfrom
ciandt-apac-dev:2.x-respect_composer_lock

Conversation

@Oscaner

@Oscaner Oscaner commented Jun 11, 2021

Copy link
Copy Markdown

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

@mcaskill mcaskill left a comment

Copy link
Copy Markdown
Contributor

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 thread src/MergePlugin.php
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
Copy Markdown
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
Copy Markdown
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

raileanunalexandru commented Jun 16, 2022 •

Copy link
Copy Markdown

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 thread src/MergePlugin.php
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
Copy Markdown
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
Copy Markdown

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