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

Regression in 0.10.4 #747

Closed
Guite opened this issue Nov 1, 2019 · 7 comments

Comments

@Guite
Copy link
Contributor

@Guite Guite commented Nov 1, 2019

After upgrading from 0.10.3 to 0.10.4 the following warning is reported by the cronjob:

PHP Warning:  count(): Parameter must be an array or an object that implements Countable in /.../lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php on line 98
PHP Stack trace:
PHP   1. {main}() /.../scripts/froxlor_master_cronjob.php:0
PHP   2. Froxlor\Cron\MasterCron::run() /.../scripts/froxlor_master_cronjob.php:20
PHP   3. Froxlor\Cron\Http\LetsEncrypt\AcmeSh::run() /.../lib/Froxlor/Cron/MasterCron.php:111
PHP   4. Froxlor\Cron\Http\LetsEncrypt\AcmeSh::needRenew() /.../lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php:113
PHP   5. count() /.../lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php:98

Corresponding line of code: https://github.com/Froxlor/Froxlor/blob/0.10.4/lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php#L98

Seems to be introduced 15 hours ago by aa85c64

@hknet

This comment has been minimized.

Copy link

@hknet hknet commented Nov 1, 2019

to compensate for both or either variable being not-countable I put this quick and dirty in to force those variables being an array added lines in /lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php

`
...
$customer_ssl = $certificates_stmt->fetchAll(\PDO::FETCH_ASSOC);

  •            if (!is_array($customer_ssl)) { 
    
  •                    $customer_ssl = array();
    
  •           }
    

...
$froxlor_ssl = Database::pexecute_first($froxlor_ssl_settings_stmt);
}

  •            if (!is_array($froxlor_ssl)) {
    
  •                    $froxlor_ssl = array();
    
  •            }
    

`

this is a quick workaround and I'm not aware of sideeffects, but this is no guarantee either.

please take this as proposed fix therefore

@Guite

This comment has been minimized.

Copy link
Contributor Author

@Guite Guite commented Nov 1, 2019

Next one:

PHP Warning:  Invalid argument supplied for foreach() in /.../lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php on line 241
PHP Stack trace:
PHP   1. {main}() /.../scripts/froxlor_master_cronjob.php:0
PHP   2. Froxlor\Cron\MasterCron::run() /.../scripts/froxlor_master_cronjob.php:20
PHP   3. Froxlor\Cron\System\TasksCron::run() /.../lib/Froxlor/Cron/MasterCron.php:111
PHP   4. Froxlor\Cron\System\TasksCron::rebuildWebserverConfigs() /.../lib/Froxlor/Cron/System/TasksCron.php:52
PHP   5. Froxlor\Cron\Http\ApacheFcgi->init() /.../lib/Froxlor/Cron/System/TasksCron.php:148
PHP   6. Froxlor\Cron\Http\LetsEncrypt\AcmeSh::run() /.../lib/Froxlor/Cron/Http/HttpConfigBase.php:37

Corresponding line of code: https://github.com/Froxlor/Froxlor/blob/0.10.4/lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php#L241

@d00p

This comment has been minimized.

Copy link
Member

@d00p d00p commented Nov 1, 2019

Yikes, that went through for me, maybe I was just lucky, I'll fix that up and get you guys 0.10.5

@d00p

This comment has been minimized.

Copy link
Member

@d00p d00p commented Nov 1, 2019

I can't seem to be able to reproduce that, might be due to my data currently, could you verify whether the following changes fix the issues?

diff --git a/lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php b/lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php
index 34cb19e0..8fc4952b 100644
--- a/lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php
+++ b/lib/Froxlor/Cron/Http/LetsEncrypt/AcmeSh.php
@@ -84,6 +84,9 @@ class AcmeSh extends \Froxlor\Cron\FroxlorCron
                                )
                ");
                $customer_ssl = $certificates_stmt->fetchAll(\PDO::FETCH_ASSOC);
+               if (!$customer_ssl) {
+                       $customer_ssl = array();
+               }
 
                $froxlor_ssl = array();
                if (Settings::Get('system.le_froxlor_enabled') == '1') {
@@ -93,6 +96,9 @@ class AcmeSh extends \Froxlor\Cron\FroxlorCron
                                (`expirationdate` < DATE_ADD(NOW(), INTERVAL 30 DAY) OR `expirationdate` IS NULL)
                        ");
                        $froxlor_ssl = Database::pexecute_first($froxlor_ssl_settings_stmt);
+                       if (!$froxlor_ssl) {
+                               $froxlor_ssl = array();
+                       }
                }
 
                if (count($customer_ssl) > 0 || count($froxlor_ssl) > 0) {
@@ -178,10 +184,10 @@ class AcmeSh extends \Froxlor\Cron\FroxlorCron
                                'id' => null
                        );
 
-                       $froxlor_ssl = $needRenew['froxlor_ssl'];
+                       $froxlor_ssl = $needRenew ? $needRenew['froxlor_ssl'] : array();
 
                        $cert_mode = 'issue';
-                       if ($froxlor_ssl) {
+                       if (count($froxlor_ssl) > 0) {
                                $cert_mode = 'renew';
                                $certrow['id'] = $froxlor_ssl['id'];
                                $certrow['expirationdate'] = $froxlor_ssl['expirationdate'];
@@ -236,7 +242,7 @@ class AcmeSh extends \Froxlor\Cron\FroxlorCron
                }
 
                // customer domains
-               $certrows = $needRenew['customer_ssl'];
+               $certrows = $needRenew ? $needRenew['customer_ssl'] : array();
                $cert_mode = 'issue';
                foreach ($certrows as $certrow) {
@Guite

This comment has been minimized.

Copy link
Contributor Author

@Guite Guite commented Nov 1, 2019

Looks good. The admin panel says there are no pending tasks anymore though. Can I somehow enforce it to run through all task types explicitly in order to verify it works properly?

@d00p

This comment has been minimized.

Copy link
Member

@d00p d00p commented Nov 1, 2019

php /var/www/froxlor/scripts/froxlor_master_cronjob.php --force --debug

If there are no tasks / no renews it's a good test too :) Else, what I just did, removed a certificate to generate a new one. So looks like this is set :)

@Guite

This comment has been minimized.

Copy link
Contributor Author

@Guite Guite commented Nov 1, 2019

So looks like this is set :)

Yep.

@d00p d00p closed this in b162324 Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.