bash script - Create virtualhost on development server (ver 2)

Question

Review compatibility, readability, good code standards, security.

This is for development server, where webmaster uses GUI fo IDE and fine way to create quickly apache2 virtualhost using GUI but fallback to text interface is present. Second review of script with applying previous review and adding more features.
I`ll post code parts fist, then whole script and git link to it.

If user runs script without terminal I need to notify him somehow about errors. xmessage provided with X but looks ugly, but looks compatible. May be suggested other way - portable and better looking. User will always get error output in terminal what's fine. In any case if using GUI user suggested to install zenity as a main gui tool of script - that will be message from xmessage.

notify_user () {
    echo "$1" >&2
    [ -t 0 ] || if type -p notify-send >/dev/null; then notify-send "$1"
    else xmessage -buttons Ok:0 -nearmouse "$1" -timeout 10; fi
}

Is following exit 1 exits function or script? I guess only function. That's why empty input not break script, at least tested in giu. How to rewrite this function to handle empty input as error to break script?

get_virtual_host() {
    if [ -t 0 ]; then
        read -p "Create virtualhost (= Folder name,case sensitive)" -r host
    else
        host=$(zenity --forms --add-entry=Name --text='Create virtualhost (= Folder name,case sensitive)')
    fi
    case "$host" in
        "")            notify_user "Bad input: empty" ;      exit 1 ;;
        *"*"*)         notify_user "Bad input: wildcard" ;   exit 1 ;;
        *[[:space:]]*) notify_user "Bad input: whitespace" ; exit 1 ;;
    esac
    echo "$host"
}

host=$(get_virtual_host)

What is better, hardcode some config string variables, allow input parameters[with leak verification], or make install script what hardcodes input parameters for server, also make .desktop file? I think hardcode is good, as one could hardcode and make permissions to edit and execute. Looks following:

webmaster="leonid"                      # user who access web files. group is www-data
maindomain=".localhost"             # domain for creating subdomains, leading dot required
serveradmin="webmaster@localhost"       # admin email
webroot="/home/${webmaster}/Web/"       # root folder where subfolders for virtualhosts created

Full script: (git: https://github.com/LeonidMew/CreateVirtualHost)

#!/bin/bash

webmaster="leonid"                      # user who access web files. group is www-data
maindomain=".localhost"             # domain for creating subdomains, leading dot required
serveradmin="webmaster@localhost"       # admin email
webroot="/home/${webmaster}/Web/"       # root folder where subfolders for virtualhosts created
a2ensite="050-"                             # short prefix for virtualhost config file
apachehost="/etc/apache2/sites-available/${a2ensite}"   # prefix for virtualhost config file
tmphost=$(mktemp)                           # temp file for edit config
trap "rm $tmphost" EXIT             # rm temp file on exit

notify_user () {
    echo "$1" >&2
    [ -t 0 ] || if type -p notify-send >/dev/null; then notify-send "$1"; else xmessage -buttons Ok:0 -nearmouse "$1" -timeout 10; fi
}

if [ -t 0 ];then
    usegui=""
else
    if ! type -p zenity >/dev/null;then
        notify_user "Use terminal or install zenity for gui. '$ sudo apt install zenity'"
        exit 1
    else
        usegui="yes"
    fi
fi

# if [ "$(id -un)" == "root" ]
# then
#    notify_user "You should not run this script as root but as user going to edit web files."
#    exit 1
# fi

get_virtual_host() {
    if [ -t 0 ]; then
        read -p "Create virtualhost (= Folder name,case sensitive)" -r host
    else
        host=$(zenity --forms --add-entry=Name --text='Create virtualhost (= Folder name,case sensitive)')
    fi
    case "$host" in
        "")            notify_user "Bad input: empty" ;      exit 1 ;;
        *"*"*)         notify_user "Bad input: wildcard" ;   exit 1 ;;
        *[[:space:]]*) notify_user "Bad input: whitespace" ; exit 1 ;;
    esac
    echo "$host"
}

host=$(get_virtual_host)

hostfile="${apachehost}${host}.conf"    # apache virtualhost config file
dir="${webroot}${host}"                 # folder used as document root for virtualhost

# virtualhost template
cat >"$tmphost" <<EOF
<VirtualHost *:80>
    ServerAdmin $serveradmin
    DocumentRoot $dir
    ServerName  ${host}${maindomain}
    ServerAlias ${host}${maindomain}
    <Directory "$dir">
        AllowOverride All
        Require local
    </Directory>
    # Available loglevels: trace8, ..., trace1, debug, info, notice, warn,
    # error, crit, alert, emerg.
    LogLevel error
</VirtualHost>
# vim: syntax=apache ts=4 sw=4 sts=4 sr noet
EOF

if [ ! -z "$usegui" ];then
    # edit virtualhost config
    text=$(zenity --text-info --title="virtualhost config" --filename="$tmphost" --editable)
    if [ -z "$text" ]
    then
        # cancel button pressed
        exit 0
    fi
    echo "$text" > "$tmphost"
else
    # edit virtualhost config
    editor=${VISUAL:-$EDITOR}
    if [ -z "$editor" ];then
        if type -p nano >/dev/null;then editor="nano"; fi
    fi
    if [ -z "$editor" ];then
        if type -p vim >/dev/null;then editor="vim"; fi
    fi
    if [ -z "$editor" ];then
        echo "edit '$tmphost' to your liking, then hit Enter"
        read -p "I'll wait ... "
    else
        "$editor" "$tmphost"
    fi
fi
# probably want some validating here that the user has not broken the config
# apache will not reload config if incorrect

getsuperuser () {
    if [ ! -z "$usegui" ];then
        echo "pkexec"
    else
        echo "sudo"
    fi
}

notify_user "execute root commands with $(getsuperuser) to create virtualhost"

$(getsuperuser) /bin/bash <<EOF
mkdir -p "$dir"
chown ${webmaster}:www-data "$dir"
chmod u=rwX,g=rX,o= "$dir"
cp "$tmphost" "$hostfile"
chown root:root "$hostfile"
chmod u=rw,g=r,o=r "$hostfile"
a2ensite "${a2ensite}${host}.conf"
systemctl reload apache2
EOF

notify_user "Virtualhost added. Apache2 reloaded."

Answer 1

Don't repeat yourself

Some snippets appear repeatedly in the code, for example:

if type -p some-command >/dev/null; then ...

Make it a function:

have_command() {
    type -p "$1" >/dev/null
}

And then you can write more naturally:

if have_command notify-send; then
    notify-send ...
fi

The same for checking if running in terminal:

in_terminal() {
    [ -t 0 ]
}

Avoid excessively long lines

It can be annoying to have to scroll to the right to see what this line is about:

[ -t 0 ] || if type -p notify-send >/dev/null; then notify-send "$1"; else xmessage -buttons Ok:0 -nearmouse "$1" -timeout 10; fi

On top of that, I think an if statement is easiest to read in this form:

if ...; then
    ...
else
    ...
fi

Lastly, I think it's hard to read when an if statement is chained after a previous command with ||.

I would find this easier to read:

in_terminal && return
if have_command notify-send; then
    notify-send "$1"
else
    xmessage -buttons Ok:0 -nearmouse "$1" -timeout 10
fi

Use more functions

This piece of code tries to detect an editor to use, by checking multiple alternatives:

editor=${VISUAL:-$EDITOR}
if [ -z "$editor" ];then
    if type -p nano >/dev/null;then editor="nano"; fi
fi
if [ -z "$editor" ];then
    if type -p vim >/dev/null;then editor="vim"; fi
fi
if [ -z "$editor" ];then
    echo "edit '$tmphost' to your liking, then hit Enter"
    read -p "I'll wait ... "
else
    "$editor" "$tmphost"
fi

What's not great about this is if the first check succeeds, the other checks still run. I mean, if EDITOR is already defined, then if [ -z "$editor" ] will still be executed 3 times for nothing.

This is a good opportunity to use a function with early returns:

find_editor() {
    local editor=${VISUAL:-$EDITOR}
    if [ "$editor" ]; then
        echo "$editor"
        return
    fi

    for cmd in nano vim; then
        if have_command "$cmd"; then
            echo "$cmd"
            return
        fi
    done
}

editor=$(find_editor)

if [ -z "$editor" ]; then
    echo "edit '$tmphost' to your liking, then hit Enter"
    read -p "I'll wait ... "
else
    "$editor" "$tmphost"
fi

Simplify conditions

This condition is written more complicated than it needs to be:

if [ ! -z "$usegui" ];then

You can simplify to:

if [ "$usegui" ]; then

Missed double-quoting

You did a good job double-quoting variables used as command arguments. Here's one place you missed:

trap "rm $tmphost" EXIT

This would be better written as:

trap 'rm "$tmphost"' EXIT