ConnectionInterface vs. SqlConnectionInterface #537

Merged
merged 2 commits into from Feb 27, 2014

Conversation

Projects
None yet
5 participants
Contributor

gharlan commented Feb 10, 2014

In many places ConnectionInterface is used as parameter type, but in method body methods of SqlConnectionInterface are used. So my IDE says that the method does not exist in ConnectionInterface.

Example:

bildschirmfoto 2014-02-10 um 12 50 19

Owner

marcj commented Feb 10, 2014

Yes, this is because it's not sure that $con is a connection to a SQL database. We probably support in the future also other databases which do maybe not have prepare() for example. A inline type hint should do it.

Member

staabm commented Feb 10, 2014

It is considered good practice to define the most generic type which is applicable for the actual use-case to increase flexibility and reduce coupling.

Contributor

gharlan commented Feb 12, 2014

Added ConnectionInterface type hints and inline type hints for SqlConnectionInterface

Member

staabm commented Feb 12, 2014

Does your change fix the warnings in the IDE?

Contributor

gharlan commented Feb 12, 2014

yes

Member

staabm commented Feb 12, 2014

So lets land it, thanks 👍

Contributor

gossi commented Feb 12, 2014

Actually, no - the code is wrong. It's correct when @staabm says "It is considered good practice to define the most generic type" BUT if you then use methods on that object from a specific subtype, you must check for that. So, either expect the specific subtype function (SqlConnectionInterface $con ..) or check:

if ($con instanceof SqlConnectionInterface) {
    // in e.g. java you would cast here
    ...
}

Some IDEs support the instanceof check and automatically adjust their type-hinting, well if not, this is where the doc-hint would make sense.

Member

staabm commented Feb 13, 2014

@gossi is right

@gharlan when the method body of a method uses methods which are only available on SqlConnectionInterface you need to use this typehint.
In case a method passes a $con to another method which requires SqlConnectionInterface we need to typehint with SqlConnectionInterface.

In places where only methods are required which are defined in ConnectionInterface then this can be used.

the phpdoc type should always match with the typehint.

Contributor

gharlan commented Feb 13, 2014

I'm not sure here because of:

We probably support in the future also other databases which do maybe not have prepare() for example.

How do you want to deal with databases which do not have these methods? Will the generated code differ depending on the database type?

Member

staabm commented Feb 13, 2014

since we need to change the typehints for such kind of thing, this would break BC and we would need a new major version anyway...

Nothing we need to think about right now IMO.. @marcj @willdurand opinions?

Owner

willdurand commented Feb 14, 2014

I don't get the problem?

Owner

marcj commented Feb 14, 2014

How do you want to deal with databases which do not have these methods? Will the generated code differ depending on the database type?

Yes, as currently.

Contributor

gharlan commented Feb 16, 2014

So, is the current pr ok, or shall I change something?

Contributor

gharlan commented Feb 26, 2014

Owner

willdurand commented Feb 26, 2014

Why not type-hinting with the SqlConnectionInterface directly? It does not make sense here.

Member

staabm commented Feb 26, 2014

People overriding this method in a subclass would get strict errors in case we change the typehint in the future from SqlConnectionInterface to ConnectionInterface...

I am not sure though if we will get non-sql connection handling in the near future and we should bother with it right now... maybe we will nevertheless have a new major version for the non-sql things and therefore can break BC anyway.

Contributor

gharlan commented Feb 26, 2014

@willdurand Where would you change the type hints? All generated methods, or only for methods using methods of SqlConnectionInterface?
Example: The doInsert method of object classes uses prepare etc., so the type hint should be SqlConnectionInterface. But doInsert is called in doSave, so the type hint should be changed there, too? And so on..

Owner

willdurand commented Feb 26, 2014

only for methods using methods of SqlConnectionInterface, that's how interfaces work. It is a contract.

Contributor

gharlan commented Feb 26, 2014

hmm, the code would look like this:

public function save(ConnectionInterface $con = null) 
{
    // ...
    $this->doSave($con);
    // ...
}

public function doSave(ConnectionInterface $con = null) 
{
    // ...
    $this->doInsert($con);
    // ...
}

public function doInsert(SqlConnectionInterface $con = null) 
{
    // ...
    $con->prepare(...);
    // ...
}

In my opinion the other methods also need SqlConnectionInterface, because they won't work with other connections. My IDE also marks this as problem.

Member

staabm commented Feb 26, 2014

yeah its a problem because you "cast" the more generic ConnectionInterface to a more specificSqlConnectionInterface implicitly.

its only works the other way arround - pass a more specific in a method which expects a more generic interface - not the way it is depicted here.

Contributor

gharlan commented Feb 26, 2014

Changed a model class like above manually.. now I get an exception on save:

Argument 1 passed to ExampleClass::doInsert() must implement interface Propel\Runtime\Connection\SqlConnectionInterface, instance of Propel\Runtime\Connection\ConnectionWrapper given.

The ConnectionWrapper implements ConnectionInterface, but not SqlConnectionInterface. But it contains all methods of SqlConnectionInterface..

Owner

willdurand commented Feb 26, 2014

Then, let's remove SqlConnectionInterface and add missing methods to the ConnectionInterface.

Contributor

gharlan commented Feb 27, 2014

Then, let's remove SqlConnectionInterface and add missing methods to the ConnectionInterface.

ok, it's ready..

@marcj marcj added a commit that referenced this pull request Feb 27, 2014

@marcj marcj Merge pull request #537 from gharlan/sql-connection-interface
ConnectionInterface vs. SqlConnectionInterface
ce3f433

@marcj marcj merged commit ce3f433 into propelorm:master Feb 27, 2014

1 check passed

default The Travis CI build passed
Details
Owner

marcj commented Feb 27, 2014

Perfect, thanks!

gharlan deleted the gharlan:sql-connection-interface branch Feb 27, 2014

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