dcarbone / php-fhir

Tools for consuming data from a FHIR server with PHP
Apache License 2.0
129 stars 40 forks source link

Autoloaded file `files/errors.php` changes application behavior #68

Closed raymondschouten closed 3 years ago

raymondschouten commented 3 years ago

I'm using this library in an Symfony application, and I noticed that my application doesn't run at all anymore after upgrading to Symfony 5.0. After debugging I found out that the culprit is the files/errors.php file that "just" replaces the PHP error handler that converts everything (notices, warnings, deprecations, etc.) to ErrorException's, which are fatal errors.

This file is automatically loaded by the Composer autoloader in the composer.json:

  "autoload": {
    "files": [
      "files/constants.php",
      "files/funcs.php",
      "files/errors.php" <---
    ],
    "psr-4": {
      "DCarbone\\PHPFHIR\\": "src/"
    }
  },

One does not expect that his application behavior (silently) is changed by just adding a Composer dependency!

dcarbone commented 3 years ago

Hmm, what was the actual issue you encountered with Symfony 5? This code has been in there for quite some time; does symfony 5 specify its own php error -> exception handler that overrides this?

raymondschouten commented 3 years ago

No, it relies on the default behavior of PHP for enabling the Zend assertions for debugging:

@ini_set('zend.assertions', 1);

Whenever the php.ini has this value configured with -1 (which is usually the default) it means that the zend.assertions cannot be enabled at runtime which emits a warning. Symfony decided to ignore the warning, as it doesn't really matter if it cannot be enabled.

But now when including the Dcarbone PHP FHIR library, the Dcarbone error handler now spits out an ErrorException instead of just a warning, which breaks the application.

I'm not saying that the way how Symfony solved this by just "silencing" the warning and ignore the outcome is the way to go, but finding out that the error handler is completely overridden globally by a vendor library just seems wrong to me. This has to be solved in another way!

And frankly I'm unable to find out why this error handler even exists. In the code base I can only find a few "catch" statements that catch \Exception specifically, and if that was the actual reason this error handler exists, then why not just checking for warnings instead?

dcarbone commented 3 years ago

if memory serves, this was added to prevent an issue presented when switching to the require_with mechanism of enforcing specific variable scoping during code generation. there are no catches for this as, within the workflow of php-fhir, these exceptions denote actual problems that must be resolved and cannot be "handled".

i agree, it could have been implemented better, but as the saying goes hindsight is 20/20.

what version are you running? i will try to get something better implemented this weekend, but if you are already in the trenches there, a PR would be welcome.

raymondschouten commented 3 years ago

I'm currently using the latest version available, and I'm unfortunately not understanding the require_with part you are describing to be able to come up with a PR...

dcarbone commented 3 years ago

its primary purpose is to enforce a smaller scope for variables defined during component template inclusion. by and large, its a wrapper around the built-in require.

dcarbone commented 3 years ago

@raymondschouten sorry, did not mean to close this.

I have created v2.0.7 with a small change where the error handler is only overridden during generation. Could you please test v2.0.7 and see if this resolves your symfony 5 compatibility issue?

raymondschouten commented 3 years ago

Thank you very much for you very quick response to my issue, your fix is simple and clean and works like a charm. Keep up the great work!