seblucas / cops

Calibre OPDS (and HTML) PHP Server : web-based light alternative to Calibre content server / Calibre2OPDS to serve ebooks (epub, mobi, pdf, ...)
http://blog.slucas.fr/en/oss/calibre-opds-php-server
GNU General Public License v2.0
1.43k stars 229 forks source link

Send To function generating NOK message #344

Closed chadberg closed 7 years ago

chadberg commented 7 years ago

I have a fully set up Send To configuration in my config_local.user.php file, and until recently it worked. With a recent pull from git I started getting NOK errors. Eventually got those to go away by commenting out "$config['cops_mail_configuration'] = NULL;" from the config_default file. I'm wondering if a recent code change has the sendtoemail function reading the default file over the local.user file?

marioscube commented 7 years ago

1 - what is a NOK message? (Never heard of it, but that doesn't mean much)

2 - just tried with a very recent version of cops and no errors.

Are you sure you did not forget a "," or something lijke that? (Happens to me sometimes)

chadberg commented 7 years ago

1- it's the error message generated if the sendtoemail function does not see properly configured email config.

2- Did you try it with a config_default.php file, a config_local.php file, and a config_local.username.php file (using http basic auth) where only the config_local.username.php file has a valid email configuration?

3- yup. Double, and triple checked. Then went ahead and copied and pasted out of the documentation, and re-did the whole thing. Then checked again.

chadberg commented 7 years ago

I think I've figured it out. In https://github.com/seblucas/cops/commit/ec4ae9a927110c753838d06f74f880553cb0243b (Oct 25) config.php was changed to set config_default and config_local to require instead of require_once. config_local.username.php was left at require_once. In https://github.com/seblucas/cops/commit/23be2810c870b9ece7faaa90293ddc805abba62f (Dec 13) sentomail was changed from only requiring config.php to requiring config.php and base.php.

base.php -also- requires config.php so the second set of requires means that config_default and config_local will override the only require_once config_local.username if they also have filled values. So requiring config and base in sequence means variables come from config_default, then config_local, then config_local.username, then config_default again then config_local again, but not config_local.username again.

marioscube commented 7 years ago

1 - I guess I can learn something new every day.

2 - not with config_local.username.php There seems to be something not working with this in the latest versions of cops.

I will try to test later today.

3 - didn't think so, but it never hurts to check.

@chadberg Hmmm that might explain the problems with config_local.username.php I have. (I hope) Thank you for finding out.

marioscube commented 7 years ago

I can confirm that changing a line in config.php from

require_once dirname(FILE) . '/' . $user_config_file; to require dirname(FILE) . '/' . $user_config_file;

fixes my problems.

@chadberg Can you confirm that it fixes your problems too?

seblucas commented 7 years ago

OK, I won't be able to check it myself before the weekend but I think this modification is correct. I would be great if either of you can make a PR, I won't forget about it that way ;)

marioscube commented 7 years ago

@chadberg

2- Did you try it with a config_default.php file, a config_local.php file, and a config_local.username.php file (using http basic auth) where only the config_local.username.php file has a valid email configuration?

I did and with the above named "fix" it works.

As a non-(recent)-programmer myself I find it a bit odd that the config files need to be referenced recursively. It looks like this is the victim of fixing problems so that it just works. (and i'm happy that it works, don't get me wrong ;-) )

chadberg commented 7 years ago

Confirmed that the fix works for me as well.

I agree that there might be a better way to do this rather than having to call the config files twice. Ideally you should be able to use require_once everywhere and never see an issue like this, but code logic is weird sometimes.

seblucas commented 7 years ago

Fixed in #345