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

Pre-load all of our sources on initial load #82

Merged
merged 1 commit into from Jun 14, 2018
Merged

Conversation

greg-1-anderson
Copy link
Collaborator

This ensures that we will not get a more recent version of one of our classes e.g. after a 'composer update' operation.

Still not sure if this is the best way, or if we should be more robust and exec every handler as considered in #79 (comment).

This is easier, and perhaps sufficient, so we'll start here. Still need to test the upgrade path.

… more recent version of one of our classes e.g. after a 'composer update' operation.
@greg-1-anderson
Copy link
Collaborator Author

Upgrades from 2.4.0 (with this PR backported) work:

$ composer require drupal-composer/drupal-scaffold:dev-test-manual-loader
./composer.json has been updated
> DrupalProject\composer\ScriptHandler::checkComposerVersion
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 0 installs, 1 update, 0 removals
  - Removing drupal-composer/drupal-scaffold (2.5.1)
  - Installing drupal-composer/drupal-scaffold (dev-test-manual-loader 7c61277): Cloning 7c61277a46 from cache
Writing lock file
Generating autoload files
> DrupalProject\composer\ScriptHandler::createRequiredFiles
$ composer update
> DrupalProject\composer\ScriptHandler::checkComposerVersion
Loading composer repositories with package information
Updating dependencies (including require-dev)
Nothing to install or update
Generating autoload files
> DrupalProject\composer\ScriptHandler::createRequiredFiles

Unfortunately, there is no way that we can actually insert this code into the older releases; everyone will need to get to 2.5.x via composer require or some other workaround. This PR will (should?) prevent this from happening in the future.

@webflo
Copy link
Member

webflo commented Jun 9, 2018

Looks could, i did some testing with the following commands and it no longer breaks.

# Install 2.4.0
composer require --no-plugins --no-scripts drupal-composer/drupal-scaffold:2.4.0 -vvv
# Upgrade to branch manual-not-auto-loader
composer require drupal-composer/drupal-scaffold:dev-manual-not-auto-loader -vvv

We could release 2.4.1 with this fix, people with trouble good upgrade to 2.4.1 first and then to 2.5.1 later.

];

foreach ($classes as $src) {
if (!class_exists('\\DrupalComposer\\DrupalScaffold\\' . $src)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is the a particular reason for the double backslash notation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The double backslash is necessary at the end; otherwise you get \', which escapes the last single-quote. I made the rest doubled up for consistency. This could be changed if you prefer it the other way.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense. Thanks!

@greg-1-anderson
Copy link
Collaborator Author

Regarding your test above, note that composer require always works, even without this PR, because the scaffold command does not run at the end of a require. This is the workaround folks will need to use to upgrade without an error. Oddly enough, it can be kind of hard to reproduce this error. I tried a couple of things and didn't see the message. It is expected that the update would work (from 2.4.x to 2.5.x) if drupal/core is not also being updated. I suspect that users who do see this error could also work around it by simply running composer update twice. I didn't try this, though.

Releasing a 2.4.1 wouldn't really make things any easier for folks. They could upgrade via composer update if they first changed their composer json to "drupal-composer/drupal-scaffold": "~2.4.0", ran composer update, edited their composer.json to put their version constraint back to ^, and then ran composer update again. However, this is no less effort than the available workarounds.

It is still very helpful to have this PR, though, as it will prevent this from happening again if the internal scaffold APIs should change again, which is likely.

];

foreach ($classes as $src) {
if (!class_exists('\\DrupalComposer\\DrupalScaffold\\' . $src)) {
Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense. Thanks!

@webflo webflo merged commit 80c7d27 into master Jun 14, 2018
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.

None yet

2 participants