[GRIDFIELD] GridFieldLevelup - Exception thrown

[phptek]

Using Raygun on our project has really given us more insight into errors that may ordinarily go unoticed, the following is a good example:

In GridFieldLevelup->getHTMLFragments() there is some bad condition checking, which should be beefed-up. The project in question is way too busy for me to consider a patch and writing tests for it, but consider the folliowing a "gist":

// DataObject::getxxxx() can of course be null (user_error()), but this is not accounted for, so we get an exception
$modelObj = DataObject::get_by_id($modelClass, $this->currentID);
if($modelObj->hasMethod('getParent')) {
    $parent = $modelObj->getParent();
} elseif(

Consider rewriting to something like this (or use an early return)

$modelObj = DataObject::get_by_id($modelClass, $this->currentID);
if($modelObj) {
    if($modelObj->hasMethod('getParent')) {
        $parent = $modelObj->getParent();
    } elseif( 
....

The Raygun exception is as follows:

Error: Call to a member function hasMethod() on a non-object
#0 Call to a member function hasMethod() on a non-object:: called at [/path/to/framework/forms /gridfield/GridFieldLevelup.php:44]
#1 RaygunLogWriter::error_handler called at [/path/to/raygun/code/RaygunLogWriter.php:71]
#2 RaygunLogWriter::shutdown_function called at [:0]

[lanacleverley]

Hi @phptek
Do you have any reproduction steps for this?

[phptek]

@whovianguitarist not off the top of my head, but hopefully you can see how a user_error() might be thrown from DataObject::get_by_id() if no result is found.

No result would be found if for some reason $modelClass were a string built dynamically in some custom logic and either dev/build hadn't been run or for some other reason $modelClass represented a non-existent ClassName.

Also, GridField->getModelClass() throws an exception which isn't actually caught in GridFieldLevelUp->getHTMLFragments().

I hope that helps, but sorry, I don't recall the specific stack trace that I based this bug report from.

[dhensby]

We should definitely be coding defensively to stop this happening, I agree. But you should also look into why you're having this problem.

The patch you suggest would be perfectly find and I would welcome a PR to that effect.

[sminnee]

Just something like this?

diff --git a/src/Forms/GridField/GridFieldLevelup.php b/src/Forms/GridField/GridFieldLevelup.php
index 84d92af19..2268644bc 100644
--- a/src/Forms/GridField/GridFieldLevelup.php
+++ b/src/Forms/GridField/GridFieldLevelup.php
@@ -62,6 +62,9 @@ class GridFieldLevelup implements GridField_HTMLProvider
 
         /** @var DataObject|Hierarchy $modelObj */
         $modelObj = DataObject::get_by_id($modelClass, $this->currentID);
+        if (!$modelObj) {
+            throw new \LogicException("Can't find object of class $modelClass ID #{$this->currentID}");
+        }
 
         $parent = null;
         if ($modelObj->hasMethod('getParent')) {

Will PR it...