Error with config-import when config sync not used

[gilzow]

Describe the bug

Line 87 of .platform.app.yaml includes a drush config-import command in the deploy hook. If the project doesn't actively use configuration management, then the config import will fail with the error included below in the logs section.

Include some logs

    W:  [error]  Drupal\Core\Config\ConfigImporterException: There were errors validating the config synchronization.
    W: This import is empty and if applied would delete all of your configuration, so has been rejected. in Drupal\Core\Config\ConfigImporter->validate() (line 750 of /app/web/core/lib/Drupal/Core/Config/ConfigImporter.php). 
    W: 
    W: In ConfigImportCommands.php line 360:
    W:                                                                                
    W:   The import failed due to the following reasons:                              
    W:   This import is empty and if applied would delete all of your configuration,  
    W:    so has been rejected.                                                       
    W:                        

Reproducing

Do not use configuration management, push change to a project environment.

Expected behavior

Only run config-import if the project is using configuration management.

Your environment

Default Drupal9 template

Screenshots

N/A

Additional context

Note, these changes are identical to #114 as they are related to one another.
Suggest changing deploy to something like the following:

    deploy: |
        set -e
        php ./drush/platformsh_generate_drush_yml.php
        cd web
        if [ -n "$(drush status bootstrap)" ]; then
          drush -y cache-rebuild
          drush -y updatedb
          if [ -n "$(ls $(drush php:eval "echo realpath(Drupal\Core\Site\Settings::get('config_sync_directory'));")/*.yml 2>/dev/null)" ]; then
            drush -y config-import
          else
            echo "No config to import. Skipping."
          fi
        else
          echo "Drupal not installed. Skipping standard Drupal deploy steps"
        fi

We get the config directory from drush, concatenate it with /*.yml and use that with ls. If there are config files, then the return from ls will not be empty so we can continue with config-import.

There was some concern mentioned about settings not being available. According to the docs for Settings::get

Return value
mixed The value of the setting, the provided default if not set.

In the absence of a setting, according to the docs, the default is

By default, Drupal places the configuration sync directory within the site's files directory, using a hash as part of the directory name, thus sites/default/files/config_HASH

if it were to return that default, the call to realpath on a non-existent path will return a false which when echo’ed will be an empty string. Returned back to the ls command, we’re lsing on / which doesnt include any yaml files.

If needed we could break it out further to exit processing if realpath returns an empty string.

[rfay]

It would be good to remove this stuff from people who use a naive workflow. 👍

[dman-coders]

The logic is good, and the problem it solves is sensible.
But I have bad feelings about this amount of script embedded in yaml files. It's suddenly become twice as complex for casual developers to parse, and easier to break things badly with yaml+bash quoting and escaping issues.
[opinion]Code embedded in yaml configs should be one-liners, not structured logic & syntax[/opinion]

I propose that this would be a suitable time to squash extract this logic into ./drush/platformsh_deploy_drupal.sh and run this little sequence that way. It's partly just aesthetic/code-style, but I think that the cleaner the yamls can be kept, the more sane they will be to support. Easier to comment out for "naive workflow" types also.
counterpoint: It DOES lower the in-your-face visibility of what we do with the drush cache:clear every time

... but I think the complexity (yaml enwrapping bash enwrapping drush enwrapping php eval) warrants being split out.

[gilzow]

I'm not married to the idea of having it in the yaml file, and share your concerns @dman-coders. We can certainly move it into its own shell script and then heavily document the yaml file to indicate why we're executing the script. I'm more interested in a sanity-check me on the logic I was suggesting. So if you're in agreement that the logic is good, I'll work on a proper PR where it's been moved over into a separate shell script.

[dman-coders]

Logic is basically good, yes, though I have seen projects that will find ways to break it.

The first common use case that this will have to be extended for is multisites. Our documented support for the drush commands in multisites is already lacking (as in, our current multisite template does not even work) but it's been easy enough to tweak until now.

In the easy cases, with only 2 or 3 multisites, I expect a developer to basically go :

drush --uri=site1 cache-rebuild
drush --uri=site1 updatedb
drush --uri=site1 config-import
drush --uri=site2 cache-rebuild
drush --uri=site2 updatedb
drush --uri=site2 config-import

Repetitive, but straightforwards.

To this end, I would add a preamble to the proposed shell script to allow it to take an arg:

if [ ! -z "$1" ]; then
  DRUSH_OPTIONS_URI="$1"
fi

So that URI definitions can be optionally invoked with :

./drush/platformsh_deploy_drupal.sh site1
./drush/platformsh_deploy_drupal.sh site2

There are other non-multisite reasons for defining the URI also . Sometimes projects - especially ones migrated from Acquia or other architectures - have been set up to define the site explicitly - it's not always default. You'd expect this to be rare, but I've seen in in close to 10% of the enterprisey sites I've audited on platform.sh, so I think it's worth including at this time.

[gilzow]

to get a list of the sites in a multisite, would the best method be to grab all the names of the subfolders in DRUPAL_ROOT/sites/*? For non multisites, I would assume we could grab the primary=true entry in routes?
Then we could just loop through the list so the user doesnt have to create an entry for each one.

[dman-coders]

Been there, done that.

./drush/drush-multisite

#!/bin/bash

# Run a given drush command over all multisites.
# Substitute for the old `drush @all` command.
# Assumes you are running from the context of docroot.

for dir in sites/*/; do
  if [ -f "$dir/settings.php" ]; then
    subsite=$(echo "$dir" | cut -f 2 -d '/') ;
    drush --uri="${subsite}" "$@"
  fi
done

I turns out that half the sites with > 3 subsites also can get a bit "unique" with their preferences, so the above script that tries to follow that simple heuristic - works half the time. Other times you find it atempting to bootstrap drush on unexpected instances like sites/localdev.otto as well.
But yes, it's a good start, or at least an example for the docs that I've been sharing with folk as they need it.

[gilzow]

I turns out that half the sites with > 3 subsites also can get a bit "unique" with their preferences, so the above script that tries to follow that simple heuristic - works half the time. Other times you find it atempting to bootstrap drush on unexpected instances like sites/localdev.otto as well.

I've dealt with that before for WordPress multisites. We can always grab the list of valid domains from routes (ie production_url) and then only use the ones that match. Let me work up a PoC and submit as a PR

[dman-coders]

Don't forget that half of the Drupal multisites out there are sub-dir-based, not subdomain based - meaning they won't show up in routes 😁

[gilzow]

i think for now I'm going to build it for single site (with an eye towards multisite) since we already have multisites as a separate template.

[gilzow]

PR created. As mentioned it doesnt include passing in the uri as I need to see an example of a single-instance site that isnt set up using default, and then some examples of multisite where things are "weird" in order to formulate a strategy to address those instances.