cv doesn't exit with an non-zero code on error

lsh-0 commented 7 years ago

in this example the civi database doesn't exist:

$ cv dl "uk.co.vedaconsulting.mosaico@https://github.com/veda-consulting/uk.co.vedaconsulting.mosaico/archive/1.0.zip"
<p>Initialization Error</p><p><pre>Array
(
    [callback] =&gt; Array
        (
            [0] =&gt; CRM_Core_Error
            [1] =&gt; simpleHandler
        )

    [code] =&gt; -24
    [message] =&gt; DB Error: connect failed
    [mode] =&gt; 16
    [debug_info] =&gt;  [nativecode=Unknown database 'cividb']
    [type] =&gt; DB_Error
    [user_info] =&gt;  [nativecode=Unknown database 'cividb']
    [to_string] =&gt; [db_error: message=&quot;DB Error: connect failed&quot; code=-24 mode=callback callback=CRM_Core_Error::simpleHandler prefix=&quot;&quot; info=&quot; [nativecode=Unknown database 'cividb']&quot;]
)
</pre></p><p></p><p>Initialization Error</p><p><pre>Array
(
    [callback] =&gt; Array
        (
            [0] =&gt; CRM_Core_Error
            [1] =&gt; simpleHandler
        )

    [code] =&gt; -24
    [message] =&gt; DB Error: connect failed
    [mode] =&gt; 16
    [debug_info] =&gt;  [nativecode=Unknown database 'cividb']
    [type] =&gt; DB_Error
    [user_info] =&gt;  [nativecode=Unknown database 'cividb']
    [to_string] =&gt; [db_error: message=&quot;DB Error: connect failed&quot; code=-24 mode=callback callback=CRM_Core_Error::simpleHandler prefix=&quot;&quot; info=&quot; [nativecode=Unknown database 'cividb']&quot;]
)
</pre></p><p></p>
$ echo $?
0

I would expect something other than 0 (success).

totten commented 7 years ago

Ah, interesting. So the ExtensionDownloadCommand (and a bunch of other cv subcommands) does check for a bunch of error-conditions and exits with non-zero for those, but the error above is a bit different. In that case, cv calls the civicrm-core bootstrap logic. That bootstrap logic doesn't report errors in a pretty way -- by default it calls an older PEAR fatal-error-handler (designed for web-based consumers).

There is a protocol for swapping the PEAR fatal-error-handler and using Exceptions instead (e.g. CRM_Core_TemporaryErrorScope::useException()). Exceptions are nice because they bubble up in a way that cv (Symfony Console) can catch the error and report it nicely. But the protocol assumes that you've already booted!

What we probably need is a patch to https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Error.php with another protocol for opting into Exception-based errors before bootstrap. (Ex: define('CIVICRM_DEFAULT_ERROR','exception');)

lsh-0 commented 7 years ago

I'm not a php/civi/drupal developer, I just need to get a growing list of civi extensions installed in an automated fashion.

I'm happy to provide any version details and test any solutions but can't contribute any code. Sorry

mfb commented 6 years ago

When outputting the error page, CiviCRM also doesn't give a proper HTTP status code :/ This could result in e.g. a webhook not retrying.

Visiting /sites/all/modules/civicrm/extern/soap.php without database outputs Initialization Error with status 200. Running cv ext:upgrade-db gives you the same Initialization Error with exit code 0.

totten commented 2 years ago

I believe the edge-case above was eventually resolved:

[bknix-max:~/bknix/build/dmaster/web/sites/all/modules/civicrm] cv ev -v 'echo 123;' ; echo "exit=[$?]"
<p>Initialization Error</p><p><pre>Array
(
    [callback] =&gt; Array
        (
            [0] =&gt; CRM_Core_Error
            [1] =&gt; simpleHandler
        )

    [code] =&gt; -24
    [message] =&gt; DB Error: connect failed
    [mode] =&gt; 16
    [debug_info] =&gt;  [nativecode=Unknown database 'dmastercivi_pa1rx']
    [type] =&gt; DB_Error
    [user_info] =&gt;  [nativecode=Unknown database 'dmastercivi_pa1rx']
    [to_string] =&gt; [db_error: message=&quot;DB Error: connect failed&quot; code=-24 mode=callback callback=CRM_Core_Error::simpleHandler prefix=&quot;&quot; info=&quot; [nativecode=Unknown database 'dmastercivi_pa1rx']&quot;]
)
</pre></p><p></p><p>Initialization Error</p><p><pre>Array
(
    [callback] =&gt; Array
        (
            [0] =&gt; CRM_Core_Error
            [1] =&gt; simpleHandler
        )

    [code] =&gt; -24
    [message] =&gt; DB Error: connect failed
    [mode] =&gt; 16
    [debug_info] =&gt;  [nativecode=Unknown database 'dmastercivi_pa1rx']
    [type] =&gt; DB_Error
    [user_info] =&gt;  [nativecode=Unknown database 'dmastercivi_pa1rx']
    [to_string] =&gt; [db_error: message=&quot;DB Error: connect failed&quot; code=-24 mode=callback callback=CRM_Core_Error::simpleHandler prefix=&quot;&quot; info=&quot; [nativecode=Unknown database 'dmastercivi_pa1rx']&quot;]
)
</pre></p><p></p>
exit=[255]

I'm not exactly sure when that was fixed. It could've been the extra shutdown function (2ba941e4164e6a8bd915be0c08877ba805be1bc4), or it could've been an improvement in civicrm-cores error-reporting during bootstrap.

On the tangential point about core and HTTP exit codes, I would note improved coverage from tests like:

civicrm / cv

cv doesn't exit with an non-zero code on error #24