Skip to content

Commit

Permalink
Throw exceptions when non Datasource classes are used as Datasources.
Browse files Browse the repository at this point in the history 
Using models as datasources can cause segmentation faults. Guard against
that by checking types and raising exceptions early.

Fixes #3694
  • Loading branch information
@markstory
markstory committed Mar 13, 2013
1 parent 73310f9 commit 3d4ebc0
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 3 deletions.
2 changes: 1 addition & 1 deletion lib/Cake/Error/exceptions.php
View file
Expand Up @@ -441,7 +441,7 @@ class MissingDatasourceConfigException extends CakeException {
*/
class MissingDatasourceException extends CakeException {

protected $_messageTemplate = 'Datasource class %s could not be found.';
protected $_messageTemplate = 'Datasource class %s could not be found. %s';

}

Expand Down
12 changes: 10 additions & 2 deletions lib/Cake/Model/ConnectionManager.php
View file
Expand Up @@ -99,9 +99,17 @@ public static function getDataSource($name) {
$conn = self::$_connectionsEnum[$name];
$class = $conn['classname'];

self::$_dataSources[$name] = new $class(self::$config->{$name});
self::$_dataSources[$name]->configKeyName = $name;
$instance = new $class(self::$config->{$name});
$instance->configKeyName = $name;

if (!$instance instanceof Datasource) {
throw new MissingDatasourceException(array(
'class' => $class,
'plugin' => null,
'message' => 'Only classes extending Datasource can be used as datasources.'
));
}
self::$_dataSources[$name] = $instance;
return self::$_dataSources[$name];
}

Expand Down
3 changes: 3 additions & 0 deletions lib/Cake/View/Errors/missing_datasource.ctp
View file
Expand Up @@ -22,6 +22,9 @@ $pluginDot = empty($plugin) ? null : $plugin . '.';
<p class="error">
<strong><?php echo __d('cake_dev', 'Error'); ?>: </strong>
<?php echo __d('cake_dev', 'Datasource class %s could not be found.', '<em>' . $pluginDot . $class . '</em>'); ?>
<?php if (isset($message)): ?>
<?php echo h($message); ?>
<?php endif; ?>
</p>
<p class="notice">
<strong><?php echo __d('cake_dev', 'Notice'); ?>: </strong>
Expand Down

10 comments on commit 3d4ebc0

@lorenzo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, that is not a very happy thing for me. I use tons of datasources that do not extend the base class :(

@ADmad
Copy link
Member

@ADmad ADmad commented on 3d4ebc0 Mar 13, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the related ticket correctly, he provided incorrect value for datasource key in config and coincidentally there was a model class loaded of the same name which got used as datasource instance. Is that right? This sounds like a pretty rare edge case.

Given the fact that web services can be pretty varied in nature I am sure there must be others like @lorenzo who have made datasource class which don't extend the base class.

@ceeram
Copy link
Contributor

@ceeram ceeram commented on 3d4ebc0 Mar 13, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for undoing this

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to try and check for things like using a model/controller/helper/component/behavior. Right now the failure case is a segmentation fault. I'm open for other ways of fixing this though. Perhaps we can sniff a method?

@ceeram
Copy link
Contributor

@ceeram ceeram commented on 3d4ebc0 Mar 13, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that would solve the issue of doing:

public $someConfg = array(
   'datasource' => 'Post'
);

And the app having a Model class, that may already be loaded.

@ADmad
Copy link
Member

@ADmad ADmad commented on 3d4ebc0 Mar 13, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about checking that if datasource's value doesn't contain a / it should end in source ?

@ceeram
Copy link
Contributor

@ceeram ceeram commented on 3d4ebc0 Mar 13, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i meant sth like:

    if (strpos(App::location($className), 'Datasource') === false) {
        throw new Exception();
    }

@ADmad
Copy link
Member

@ADmad ADmad commented on 3d4ebc0 Mar 13, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ceeram That sounds promising.

@ceeram
Copy link
Contributor

@ceeram ceeram commented on 3d4ebc0 Mar 13, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a pull request: #1176

Please sign in to comment.