Start to use SPL Exceptions #90

Merged
merged 21 commits into from Dec 19, 2011

2 participants

@willdurand
Propel member

Hi there,

I started to add named exceptions on Propel (mostly Runtime). More and more new code appears and we don't use specific exceptions. To avoid answers like "this is not the purpose of this PR", I decided to work on it.
Once we'll add named exceptions, people will be encouraged (forced) to use named exceptions. Diabolic strategy, right? :D

Anyway, here is a first work. Before to continue I need feedback. I removed the workaround on the PropelException. Oh, and I kept PropelException when I had no idea about what to put instead.

SPL exceptions: http://php.net/manual/fr/spl.exceptions.php

William

@fzaninotto fzaninotto and 1 other commented on an outdated diff Dec 13, 2011
src/Propel/Runtime/Adapter/Pdo/PdoAdapter.php
@@ -41,7 +41,7 @@ public function getConnection($conparams)
$conparams = $this->prepareParams($conparams);
if (!isset($conparams['dsn'])) {
- throw new PropelException('No dsn specified in your connection parameters');
+ throw new \InvalidArgumentException('No dsn specified in your connection parameters');
@fzaninotto
Propel member

why not declare the exception in a use statement to get rid of the backslash, just like in the previous file?

@willdurand
Propel member

This is my question actually. Should we add use statements every time ? (I don't have any problem about doing that)

@willdurand
Propel member

My decision is to use use statements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fzaninotto fzaninotto and 1 other commented on an outdated diff Dec 13, 2011
src/Propel/Runtime/Adapter/Pdo/SqlsrvAdapter.php
@@ -13,6 +13,7 @@
use Propel\Runtime\Adapter\AdapterInterface;
use Propel\Runtime\Connection\ConnectionInterface;
use Propel\Runtime\Connection\StatementInterface;
+use Propel\Runtime\Exception\UnsupportedEncodingException;
@fzaninotto
Propel member

I'd namespace this exception Propel\Runtime\Adapter\Pdo\UnsupportedEncodingException. Let's be specific.

@willdurand
Propel member

Yeah, you're right, but I'd like to use Propel\Runtime\Adapter\Exception\UnsupportedEncodingException

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fzaninotto fzaninotto and 1 other commented on an outdated diff Dec 13, 2011
src/Propel/Runtime/Collection/ArrayCollection.php
@@ -31,7 +31,7 @@ class ArrayCollection extends Collection
public function save($con = null)
{
if (!method_exists($this->getModel(), 'save')) {
- throw new PropelException('Cannot save objects on a read-only model');
+ throw new \BadMethodCallException('Cannot save objects on a read-only model');
@fzaninotto
Propel member

I'd extend \BadMEthodCall, but inside a Propel subnamespace. Something like:

<?php
use BadMethodCalExceptionl;

class Propel\Runtime\Exception\BadMethodCallException extends BadMethodCallException
{
}

I think all our exceptions should somehow be Propel exceptions. Maybe we can even add an empty PropelExceptionInterface exception (dunno if it makes sense).

@willdurand
Propel member

Agreed to extend core exceptions.
But I can't be agree with you on the PropelExceptionInterface.

@willdurand
Propel member

hmm, we could have exception interface for the adapter part (for PDOException for instance)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willdurand
Propel member

Added more commits, I like how it's becoming :)

@willdurand
Propel member

Here we go, all classes in the Runtime have named exceptions. There are still some PropelException but it will be changed later.

What I did and what I'd like we start to use is:

  • Add common exceptions in Propel\Runtime\Exception
  • Add specific exceptions in a Exception subnamespace
  • Never use core exceptions
  • Common exceptions extend core exceptions
  • Naming has to be clear/explicit

Naming conventions:

  • <Object>NotFoundException
  • Unsupported<Something>Exception
  • Unknown<Object>Exception
@willdurand
Propel member

(rebased)

@willdurand
Propel member

@fzaninotto @jaugustin if there is no concern, then I'll merge it.

@willdurand willdurand merged commit a22555f into propelorm:master Dec 19, 2011
@fzaninotto
Propel member

Awesome. Great work!

@fzaninotto
Propel member

Just one question. What's the point of extending \Propel\Runtime\Exception if not all exceptions extend it? Sometimes your exceptions extend Propel Exception, sometimes a core SPL exception. Not sure if a developer can take advantage of that.

@willdurand
Propel member

We should move PropelException to Exception.

@fzaninotto
Propel member

Does that mean that there will be no common denominator to all Propel exceptions (except the namespace)? If so, since a namespace is not catchable, I'm think we're not going far enough.

Why not consider an exception interface, which could be added to all Propel exceptions and allow us to have both type and context?

@willdurand
Propel member

The common denominator is \Exception. Each (in use) core exception has a Propel equivalent, in Propel\Runtime\Exception. Each specialized exception extends one of these classes.

So, what do you want ? An interface could be fine on some parts (adapters for instance), but do we need to add an interface that covers all exceptions ? I don't see a use case there.

since a namespace is not catchable

What do you want to say ?

@fzaninotto
Propel member

Suppose I'm a developer, and in my app I want to catch all Propel Exceptions. This used to be possible in Propel 1. It's not possible anymore, since there is no common denominator.

So I suggest we introduce a

<?php
namespace Propel\Runtime;
interface PropelException {}

And that all runtime exceptions implement it. That way, the developer can simply catch Propel\runtime\PropelException to catch all Propel exceptions.

Note that this should be tested first, I'm not sure that catch is sensible to interfaces...

@willdurand
Propel member

Agreed. In a perfect world, we just have to add this interface on classes in Propel\Runtime\Exception.

Oh, the interface name is wrong : ExceptionInterface seems better. And yes, don't know if an interface is catchable..

@fzaninotto
Propel member

Tested, and it works.

https://gist.github.com/1502867

@willdurand
Propel member

Good to know, I'm working on that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment