Problem/Motivation

The current instructions for updating core can result in broken sites.

3. Remove the 'core' and 'vendor' directories. Also remove all of the files
in the top-level directory, except any that you added manually.

If you made modifications to files like .htaccess, composer.json, or
robots.txt you will need to re-apply them from your backup, after the new
files are in place.

5. Re-apply any modifications to files such as .htaccess, composer.json, or
robots.txt.

If you've done the following your site will be broken:

  1. Install Drupal from git or the tarball
  2. Get a module like AMP or Address that uses a library
  3. Follow the instructions to do composer require from root
  4. Follow the core UPDATE.txt instructions

If you re-apply the changes to composer.json by hand then vendor won't be magically fixed until you've run composer install. If you maintained all the non core provided directories in vendor this won't work either because the autoloader in vendor/composer will not know how to get to the non core dependencies.

Proposed resolution

I'm not sure what the real instructions should be. Here are some thoughts:

  1. We need to ensure hook_requirements detects missing composer dependencies and is fired before updating (drush needs a fix for this https://github.com/drush-ops/drush/pull/2708 and not all modules do it correctly #2867687: We should be checking if the lullabot/amp is available during update.php too) See: #2494073: Mark modules with unmet composer dependencies uninstallable
  2. We should be telling people to do "composer require dependency" again this ensures that the latest version and any dependency conflicts etc are detected
  3. What about other composer.json customisations?
  4. We should tell people if drupal/core is in the require section of their project (ie. they have a composer managed project) to just do composer update drupal/core --with-dependencies

Remaining tasks

User interface changes

API changes

Data model changes

Comments

alexpott created an issue. See original summary.

cilefen’s picture

Issue tags: +composer
alexpott’s picture

Status: Active » Needs review
FileSize
3.39 KB

Some work in progress.

Status: Needs review » Needs work

The last submitted patch, 3: 2867757-3.patch, failed testing.

Mile23’s picture

Added #2494073: Mark modules with unmet composer dependencies uninstallable to IS.

Also, if you say composer update you'd rightly expect the listed dependencies to be updated. But they won't be, because composer-installer puts Drupal-flavored stuff (including their composer.json files) all over the file system beyond where composer expects it to be. That's fine for initially building a codebase based on an existing composer.json file, but breaks composer when its time to update dependencies. That's why this exists: https://github.com/paul-m/drupal-merge-plugin

Basically the reason its hard to document is because it's broken.

Here's what happens when you follow these instructions on a new installation of Drupal 8.2.x (after running composer install):

  1. +++ b/core/UPDATE.txt
    @@ -64,7 +64,21 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +3. Determine if your project is composer managed project. You can do this by
    +   running "composer info drupal/core" from the command line. If this is
    
    $ composer info drupal/core
                                     
      [InvalidArgumentException]     
      Package drupal/core not found  
    
  2. +++ b/core/UPDATE.txt
    @@ -64,7 +64,21 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +   the command line in the root directory and skip to step 7. Replace X and Y
    

    X and Y where?

  3. +++ b/core/UPDATE.txt
    @@ -64,7 +64,21 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +     composer update drupal/core --with-dependencies
    
    $ composer update drupal/core --with-dependencies
    Package "drupal/core" listed for update is not installed. Ignoring.
    Loading composer repositories with package information
    Updating dependencies (including require-dev)
    [etc]
    
kylebrowning’s picture

I would like to point out that while its not an official Drupal project by any means, BLT handles this for you.

alexpott’s picture

Status: Needs work » Needs review
FileSize
623 bytes
3.31 KB

@Mile23
1. Precisely - you don't have a composer managed project. You have a git managed project at the point. You did not object drupal/core via composer.
2. That was left in by mistake
3. See point 1.

These instructions will work if you have a project created using composer by following the instructions on https://www.drupal.org/docs/develop/using-composer/using-composer-with-d... and go the Drupal project root.

@Mile23 by

because composer-installer puts Drupal-flavored stuff (including their composer.json files) all over the file system

I think you don't mean composer-installer - I think you are talking about https://www.drupal.org/project/composer_manager which as the project page says is:

The Drupal 8 version of this module is deprecated and no longer needed, due to improvements in Drupal 8.1. Use Composer directly to get the needed modules, which will also download their required libraries.

anavarre’s picture

  1. +++ b/core/UPDATE.txt
    @@ -64,7 +64,20 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +3. Determine if your project is composer managed project. You can do this by
    

    How about "Determine if your project is managed by Composer."?

  2. +++ b/core/UPDATE.txt
    @@ -64,7 +64,20 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +   successful, you have a composer managed project. If you don't have composer
    

    composer managed or composer-managed?

  3. +++ b/core/UPDATE.txt
    @@ -64,7 +64,20 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +   successful, you have a composer managed project. If you don't have composer
    

    Seems Composer takes a capital 'C' when we're talking about the project rather than the binary (e.g. page title/footer on getcomposer.org)

    Also, should we say composer-managed instead? If yes, would the capital 'C' not apply (since in this instance it could be because we're referring to the binary)

  4. +++ b/core/UPDATE.txt
    @@ -64,7 +64,20 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +   it will be present in the "replace" section).
    

    I'm not sure about that. You could have "drupal/core" added under the "require" section only, e.g. https://github.com/anavarre/drucker/blob/master/orchestration/files/comp...

    So perhaps it's either/or?

  5. +++ b/core/UPDATE.txt
    @@ -64,7 +64,20 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +   If the project is not managed by composer skip to step 4.
    

    Capital 'C'?

  6. +++ b/core/UPDATE.txt
    @@ -64,7 +64,20 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +   If the project is a composer managed project run the following command from
    

    Perhaps "If the project is managed by Composer"

  7. +++ b/core/UPDATE.txt
    @@ -64,7 +64,20 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +     composer update drupal/core --with-dependencies
    

    Since it's a new line, should we add a '$' for the command prompt?

  8. +++ b/core/UPDATE.txt
    @@ -113,10 +126,15 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +6. Re-apply any modifications to files such as .htaccess or robots.txt.
    

    For composer-managed projects, should we mention drupal-scaffold's 'excludes' directive to prevent people from having to remember that .htaccess or robots.txt can be ovewritten?

  9. +++ b/core/UPDATE.txt
    @@ -113,10 +126,15 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +     composer require drupal/address
    

    Same as above, should we add '$' here?

Mile23’s picture

Status: Needs review » Needs work

I think you don't mean composer-installer - I think you are talking about https://www.drupal.org/project/composer_manager which as the project page says is:....

No, I'm talking about the use case documented here: https://www.drupal.org/node/2822349

This is actually the main use case you'd want if you've got a codebase and want to start managing it with composer, as you might if you're using pantheon or whatever and you ended up needing drupal/address. Combine its solution with the subtree split of core, and we're not worried about how to document this any more, because it's reasonable.

Setting to needs work because of the review in #8

alexpott’s picture

@Mile23 well https://www.drupal.org/docs/develop/using-composer/managing-dependencies... needs an update with information from https://www.drupal.org/docs/develop/using-composer/using-composer-with-d...

If the steps on https://www.drupal.org/docs/develop/using-composer/managing-dependencies... are following you are not managing you drupal/core dependency with composer and the new advice in step 6 is directly relevant to you:

+++ b/core/UPDATE.txt
@@ -113,10 +126,15 @@ following the instructions in the INTRODUCTION section at the top of this file:
+   If you have changes to composer.json it is recommended that you re-install
+   the dependencies instead of applying the changes by hand. For example, to
+   reinstall the Address module and its dependencies run:
+
+     composer require drupal/address
alexpott’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
3.35 KB

Thanks review @anavarre.

1. Fixed
2. I've made this consistent to "project managed by Composer"
3. Capitalised it everywhere
4. I've removed this. "drupal/core" is ALWAYS present in the replace section. But if it is present in the require section you have a project where drupal is being managed by Composer
5. Fixed
6. Fixed
7. This would not be consistent with the rest of the file
8. Not sure - can you try to write this.
9. See 7.

anavarre’s picture

Status: Needs review » Needs work
  1. +++ b/core/UPDATE.txt
    @@ -64,16 +64,20 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +3. Determine if your project is managed by Composer. To determine this run the
    

    Could be simplified to "by running the following command:" and would prevent repeating "determine" twice.

  2. +++ b/core/UPDATE.txt
    @@ -64,16 +64,20 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +   Composer installed and "drupal/core" is present "require" section of
    

    How about:

    in the "require" section of your composer.json file, then the project is managed by Composer and will require you to install the Composer executable in order to update it.

re: #7 perhaps something like:

Alternatively, when Drupal is managed by Composer https://github.com/drupal-composer/drupal-scaffold can help you with excluding files you'd like to preserve always when updating Drupal.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
3.37 KB

Thanks again @anavarre

Patch addresses 1 and 2.

Looking thinking about drupal-scaffold - I think this is the wrong point for this. We should update the composer docs to mention it maybe but it's a part of https://github.com/drupal-composer/drupal-project so it might be there for the majority of composer managed projects anyway. I think it just muddies the water here.

anavarre’s picture

Status: Needs review » Reviewed & tested by the community

We should update the composer docs to mention it

It's mentioned at https://www.drupal.org/docs/develop/using-composer/using-composer-to-man... so it seems we're good here.

As far as I'm concerned the patch looks good now (assuming it comes back green).

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
+   If the project is managed by Composer, run the following command from the
+   command line in the root directory and skip to step 7.

Step #4 includes things like looking at the release notes and seeing if any manual changes need to be made to settings.php. Why would those be skipped?

+3. Determine if your project is managed by Composer by running the following
+   command:
+
+     composer info drupal/core

This assumes a global installation of composer. But if you follow the basic instructions at https://getcomposer.org you wind up with a local install instead, and the command is php composer.phar info drupal/core. Maybe at least list both of those as alternatives?

It may actually be better to just put something in the Status Report that tells you if your project is managed by Composer (it should be very easy to have code in Drupal that parses composer.json and checks whether "drupal/core" is present in the "require" section or not). Then it would not be necessary to mess with the command line to figure it out. Could be a separate issue.

alexpott’s picture

@David_Rothstein thanks for the review - good points. I've tried to address them all by re-organising the file and replace composer with PATH/TO/composer. I think the new organisation separates the concerns of updating your code - which is different depending on whether you use composer or not and updating the database (which is not different).

alexpott’s picture

Oops included my yarn.lock file....

The last submitted patch, 16: 2867757-16.patch, failed testing.

anavarre’s picture

Even better. I'll leave it up to @David_Rothstein to RTBC, though.

xjm’s picture

Status: Needs review » Needs work
  1. +++ b/core/UPDATE.txt
    @@ -64,7 +64,22 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +3. Determine if your project is managed by Composer by running the following
    +   command (replace /PATH/TO with the appropriate location for your system):
    

    This assumes the user has the ability or knowledge to "run a command", which is not part of the current expectations for users who install from a tarball.

  2. +++ b/core/UPDATE.txt
    @@ -64,7 +64,22 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +   If this is successful, your project is managed by Composer. If you don't have
    +   Composer installed and "drupal/core" is present in the "require" section of
    +   your composer.json file, then the project is managed by Composer. You need to
    +   install the Composer executable in order to update it.
    

    This could be misread as telling people who don't have Composer that they need to install it.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.57 KB
4.86 KB

Thanks for the review @xjm

Re #1 - this is a copy&paste kinda from things that already exist in INSTALL.txt - for example:

   On a typical Unix/Linux command line, use the following commands to download
   and extract:

     wget https://www.drupal.org/files/projects/drupal-x.y.z.tar.gz
     tar -zxvf drupal-x.y.z.tar.gz

   This creates a new directory drupal-x.y.z/ containing all Drupal files and
   directories. Copy the files into your Drupal installation directory:

     cp -R drupal-x.y.z/* drupal-x.y.z/.htaccess /path/to/your/installation

I've tried to update the language to reflect the existing text.

Patch attached also tries to fix #2. I've also fixed a bug that is more apparent after this change - we tell people to use ftp before changing .htaccess etc...

alexpott’s picture

Issue tags: +Triaged D8 critical

Discussed with @catch, @Cottser, @effulgentsia, and @xjm. We agreed to keep this critical for now. Whilst documentation criticals are rare and contentious the current instructions can result in a broken site. There are many concerns about core's usage of composer and how different types of users are supposed to cope - see #2477789: Use composer to build sites , #2002304: [META] Improve Drupal's use of Composer and #2845379: Do not leave non-experts behind: do not require Composer unless a GUI is also included

alexpott’s picture

Also there's a big discussion on https://groups.drupal.org/node/516571 about this problem too

David_Rothstein’s picture

I like the new file organization. But:

  1. With the patch applied, the information about looking at the release notes and seeing if any manual changes need to be made to settings.php is still missing from the "UPDATING CODE WITH COMPOSER" section (it's only in "UPDATING CODE WITHOUT COMPOSER") so the issue I raised earlier still needs to be addressed.

    Possibly it would be better to have a separate section about applying manual changes that is after the "updating code" sections, since a lot of the information would be duplicated otherwise.

  2. +   On a typical Unix/Linux command line, this can be determined by running the
    +   following command (replace /PATH/TO with the appropriate location for your
    +   system):
    +
    +     /PATH/TO/composer info drupal/core
    

    I think it should say "replace /PATH/TO/composer with the appropriate location" instead, since as mentioned earier a default basic installation of Composer will actually have the executable named "composer.phar" (not "composer").

  3. +2. Determine if any modifications to files such as .htaccess or robots.txt and
    +   re-apply them.
    

    Missing some words - should be "Determine if there are any modifications" instead?

  4. +   excluding files you'd like to preserve always when updating Drupal.
    

    "always preserve"

Mile23’s picture

Status: Needs review » Needs work

Needs work per #24.