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 misleading "Your composer.json file was modified by BLT" error message #1139

Merged
merged 1 commit into from
Feb 28, 2017
Merged

Fix misleading "Your composer.json file was modified by BLT" error message #1139

merged 1 commit into from
Feb 28, 2017

Conversation

TravisCarden
Copy link
Contributor

If BLT modifies your composer.json file during an upgrade, it currently emits the following message:

Your composer.json file was modified by BLT, you MUST run "composer update" to update your composer.lock file.

This is misleading, because composer update does not update your composer.lock file--it upgrades all your dependencies, which would completely negate the effect of upgrading with composer update acquia/blt --with-dependencies. What is actually wanted is composer update --lock, which does just update your composer.lock file.

@grasmash
Copy link
Contributor

grasmash commented Feb 27, 2017

@TravisCarden Sometimes, running composer update --lock will not work.

For instance:

  • You run composer update acquia/blt --with-dependencies
  • BLT adds a new package to your require-dev array
  • You run composer update --lock
  • The new package is not installed

Here's another problematic scenario that isn't even BLT specific:

  • You run composer update acquia/blt --with-dependencies
  • The newest version of BLT has a conflict with another dependency of yours
  • Composer doesn't update BLT, and also doesn't throw an error.
  • BLT is not updated

IMO this is a problem with composer. Composer basically needs a --update-conflicting-sibling-packages kind of argument.

Unfortunately, the only foolproof way to ensure that BLT actually installs all of the intended packages is to run composer require acquia/blt:^[latest-release] --no-update && composer update.

How do you feel about updating the docs to reflect this?

On another note, I don't really like that BLT modifies composer.json for you. But, I don't know of a better way to accomplish the following:

  • Ship "require-dev" dependencies. This is necessary if we want to provide testing tools like Behat and PHPUnit with BLT projects, but not package them for prod.
  • Ship default "require" dependencies that are optional.
  • Ship default "extra" configuration

@TravisCarden
Copy link
Contributor Author

Okay, @grasmash. Then why don't we just update the message to reflect the real intent, so it doesn't look like a mistake?

Your composer.json file was modified by BLT, you MUST run "composer update" to re-process and update dependencies.

PR updated.

@grasmash
Copy link
Contributor

@TravisCarden yeah we can do that, but I also think a docs change is in order.

@danepowell
Copy link
Contributor

I might prefer that we recommend using plain old composer update at all times (never specifically whitelisting acquia/blt or using --with-dependencies), and then point out that if this didn't update BLT it's likely because of conflicting dependencies. Just for consistency, and so folks only have to learn a single command.

Obviously we also need to caveat that this will update any Drupal modules or other packages at the same time. My own practice is to update any individual packages before updating BLT, so that I can at least track the changes independently.

@TravisCarden
Copy link
Contributor Author

I definitely wondered what the point was of updating BLT only just so we could turn around and update everything. An alternative line of questioning is to ask why we would modify the composer.json manually in the first place (thus necessitating the second composer update) rather than using the CLI to add new dependencies (which would perform the necessary re-parsing automatically). Then the lock file would never get out of sync in the first place.

@grasmash
Copy link
Contributor

grasmash commented Feb 28, 2017

@TravisCarden I don't think I understand the approach you're proposing.

An alternative line of questioning is to ask why we would modify the composer.json manually in the first place (thus necessitating the second composer update) rather than using the CLI to add new dependencies (which would perform the necessary re-parsing automatically)

How would BLT use the CLI to add new dependencies? Are you suggesting that BLT not ship any dev dependencies, and instead instruct users to manually add them on their own? Or, are you suggesting that BLT should create new PHP processes to execute shell commands of its own during a composer update ?

@grasmash
Copy link
Contributor

grasmash commented Feb 28, 2017

I'm going to spitball a few different approaches:

  1. When composer update is called on the acquia\blt package, prompt the user "BLT has updates for your composer.json file. To apply these updates, Composer must re-solve your dependency tree. Do you want to apply these automatically?" If yes, then BLT modifies composer.json and then executes composer update afterwards (effectively running composer update twice). If --no-interaction is used, BLT relies on value of extras.blt.update to answer this question.
  2. Modify BLT's Composer plugin logic such, rather than the current "modify composer.json and require a manual composer update" approach, it dynamically requires and updates various packages during the initial composer update call. This might be really hard to do depending on Composer's APIs, which are poorly documented.
  3. Remove all automated updates to composer.json and create a new blt composer:json:update command that must be run manually. Add a message to composer update output indicating when this command needs to be run.

Just to recap, here are the requirements for BLT as a tool:

Provider non-overridable configuration for:

  • require

Provide default, overridable Composer configuration for:

  • require
  • require-dev (root-only)
  • repositories (root-only)
  • extra

Additionally, we need a mechanism to update the default values for these configuration items when there are upstream changes.

@grasmash
Copy link
Contributor

Eh, I opened #1140 to address this. Seems like a better approach than my other ideas.

@grasmash grasmash merged commit d48bf35 into acquia:8.x Feb 28, 2017
@TravisCarden TravisCarden deleted the composer-message branch February 28, 2017 18:21
@danepowell
Copy link
Contributor

I'm not too picky about how BLT updates composer.json, my only concern is that we consistently tell folks to use composer update (without any arguments), since the more "clever" approaches that we used to recommend (composer update --with-dependencies acquia/blt) can have inconsistent and utterly baffling results.

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.

3 participants