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

Catch statement in service manager should handle any exception code #26

Closed
weierophinney opened this issue Dec 31, 2019 · 11 comments
Closed

Comments

@weierophinney
Copy link
Member

Hello ;

I've a service factory like this:

/**
 * Class DatabaseServiceFactory
 * @package iMSCP\Service
 */
class DatabaseServiceFactory implements FactoryInterface
{
    /**
     * Create database service
     *
     * @param ServiceLocatorInterface $serviceLocator
     * @return Database
     */
    public function createService(ServiceLocatorInterface $serviceLocator)
    {
        $config = Registry::get('config');
        $imscpDbKeys = new ConfigFileHandler($config['CONF_DIR'] . '/imscp-db-keys');

        if (!isset($imscpDbKeys['KEY']) || !isset($imscpDbKeys['IV'])) {
            throw new \RuntimeException('imscp-db-keys file is corrupted.');
        }

        $db = Database::connect(
            $config['DATABASE_USER'],
            Crypt::decryptRijndaelCBC($imscpDbKeys['KEY'], $imscpDbKeys['IV'], $config['DATABASE_PASSWORD']),
            $config['DATABASE_TYPE'],
            $config['DATABASE_HOST'],
            $config['DATABASE_NAME']
        );

        $db->execute('SET NAMESS `utf8`');

        return $db;
    }
}

Here, if an error occurs, a PDOException is throw. The problem is that the underlying catch statement from the service manager don't handle PDOException code and thus, the following fatal error is raised:

Fatal error: Wrong parameters for Exception([string $exception [, long $code [, Exception $previous = NULL]]]) in /var/cache/imscp/packages/vendor/zendframework/zend-servicemanager/src/ServiceManager.php on line 950

Note: Here the wrong query SET NAMESS is intentional.


Originally posted by @nuxwin at zendframework/zend-servicemanager#41

@weierophinney
Copy link
Member Author

Whole story to this point is writte in the first comment here: http://php.net/manual/en/class.pdoexception.php

Question to be asked is:
Should ZF ensure a type-casting to int on all exceptions? (who knows where people might end up using PDO)? I don't think that an cast to int hurts performance much and everyplace we do that'd be an error case anyways so performance doesnt matter.


Originally posted by @manuakasam at zendframework/zend-servicemanager#41 (comment)

@weierophinney
Copy link
Member Author

@manuakasam

As I've said on IRC, I'll not loose my time with PHP dev team by reporting bugs on their bug tracker due to their EGO (and so on...).

For the record

In short: The PHP dev team does not hesitate to violate its own interfaces (by laziness?)


Originally posted by @nuxwin at zendframework/zend-servicemanager#41 (comment)

@weierophinney
Copy link
Member Author

(by laziness?)

Not really, it's mostly because many of the core members of PHP-DEV are still attached to very old practices that aren't really considered "good" anymore.


Originally posted by @Ocramius at zendframework/zend-servicemanager#41 (comment)

@weierophinney
Copy link
Member Author

This looks like it has been forgotten. Is this still an issue to resolve?


Originally posted by @spengilley at zendframework/zend-servicemanager#41 (comment)

@weierophinney
Copy link
Member Author

@spengilley

You mean in ZF3? I don't know. Even through, we can handle this problem locally by catching the exception.


Originally posted by @nuxwin at zendframework/zend-servicemanager#41 (comment)

@weierophinney
Copy link
Member Author

@nuxwin Thanks for responding.

I was talking specifically in ZF2.

I am looking for some easy issues to fix. I am new to contributing to open source :)


Originally posted by @spengilley at zendframework/zend-servicemanager#41 (comment)

@weierophinney
Copy link
Member Author

@spengilley

Not the best issue to start ;) From my point of view, this issue can be closed.

@Ocramius ?


Originally posted by @nuxwin at zendframework/zend-servicemanager#41 (comment)

@weierophinney
Copy link
Member Author

Why would you catch any exceptions in the service manager and re-throw generic, useless exceptions instead of leaving the proper exceptions to bubble up and be caught as needed by the user? If I had a penny for everytime the useless exceptions from the service manager were thrown and I had no clue what went wrong, I'd be rich. Maybe you should reconsider this in the future.


Originally posted by @lucian303 at zendframework/zend-servicemanager#41 (comment)

@weierophinney
Copy link
Member Author

You can rely on this type from a caller in order to wrap the DIC and write fallback logic.

If everything was a \Exception we may as well disable the exception system overall 😛

If the problem is dealing with "previous exceptions", then my suggestions is to get up to speed with that concept, and quickly, since most good OO libraries out there do wrap exceptions in known failure scenarios.


Originally posted by @Ocramius at zendframework/zend-servicemanager#41 (comment)

@weierophinney
Copy link
Member Author

Anyway, if anybody wants to pick this up, write a test with a factory that does something like this:

(function (Potato $foo) {})();
```

Then verify that the type of the error is indeed a `ServiceNotCreated`.

---

Originally posted by @Ocramius at https://github.com/zendframework/zend-servicemanager/issues/41#issuecomment-411298426

@GeeH
Copy link
Contributor

GeeH commented Jun 8, 2020

I'm closing this. It contains personal attacks on contributors to the PHP project which is never acceptable. Plus the original author states: "Not the best issue to start ;) From my point of view, this issue can be closed."

@GeeH GeeH closed this as completed Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants