FIX ModelAdmin::$model_importers alias support

Hasn't been considered when introducing aliases rather than class names in $managed_models. Fixed a regression from when we introduced $modelTab. This is backwards compatible since $modelTab falls back to $modelClass. Also improved $model_importers to fall back to the default per model, rather than requiring definition of *all* models on a non-null value.
silverstripe · Jun 25, 2021 · 81cf409 · 81cf409
1 parent bd8b138
commit 81cf409
Show file tree

Hide file tree

Showing 4 changed files with 106 additions and 40 deletions.
diff --git a/code/ModelAdmin.php b/code/ModelAdmin.php
@@ -2,33 +2,34 @@
 
 namespace SilverStripe\Admin;
 
-use SilverStripe\Control\Controller;
-use SilverStripe\Control\HTTPRequest;
-use SilverStripe\Control\HTTPResponse;
+use SilverStripe\Forms\Form;
 use SilverStripe\Core\Convert;
+use SilverStripe\ORM\ArrayList;
 use SilverStripe\Dev\BulkLoader;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\View\ArrayData;
+use SilverStripe\Dev\CsvBulkLoader;
 use SilverStripe\Dev\Deprecation;
-use SilverStripe\Forms\CheckboxField;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\FileField;
-use SilverStripe\Forms\Form;
 use SilverStripe\Forms\FormAction;
+use SilverStripe\Forms\HiddenField;
+use SilverStripe\Security\Security;
+use SilverStripe\Control\Controller;
+use SilverStripe\Forms\LiteralField;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Forms\CheckboxField;
+use SilverStripe\Control\HTTPResponse;
+use SilverStripe\ORM\ValidationResult;
 use SilverStripe\Forms\GridField\GridField;
 use SilverStripe\Forms\GridField\GridFieldConfig;
-use SilverStripe\Forms\GridField\GridFieldConfig_RecordEditor;
+use SilverStripe\Forms\GridField\GridFieldPaginator;
 use SilverStripe\Forms\GridField\GridFieldDetailForm;
+use SilverStripe\Forms\GridField\GridFieldPrintButton;
 use SilverStripe\Forms\GridField\GridFieldExportButton;
 use SilverStripe\Forms\GridField\GridFieldFilterHeader;
 use SilverStripe\Forms\GridField\GridFieldImportButton;
-use SilverStripe\Forms\GridField\GridFieldPaginator;
-use SilverStripe\Forms\GridField\GridFieldPrintButton;
-use SilverStripe\Forms\HiddenField;
-use SilverStripe\Forms\LiteralField;
-use SilverStripe\ORM\ArrayList;
-use SilverStripe\ORM\DataObject;
-use SilverStripe\ORM\ValidationResult;
-use SilverStripe\Security\Security;
-use SilverStripe\View\ArrayData;
+use SilverStripe\Forms\GridField\GridFieldConfig_RecordEditor;
 
 /**
  * Generates a three-pane UI for editing model classes, tabular results and edit forms.
@@ -103,14 +104,16 @@ abstract class ModelAdmin extends LeftAndMain
     /**
      * Change this variable if you don't want the Import from CSV form to appear.
      * This variable can be a boolean or an array.
-     * If array, you can list className you want the form to appear on. i.e. array('myClassOne','myClassTwo')
+     * If array, you can list {@link $managed_models} keys
+     * you want the form to appear on. i.e. array('myClassOne','myClassTwo')
      */
     public $showImportForm = true;
 
     /**
      * Change this variable if you don't want the gridfield search to appear.
      * This variable can be a boolean or an array.
-     * If array, you can list className you want the form to appear on. i.e. array('myClassOne','myClassTwo')
+     * If array, you can list {@link $managed_models} keys
+     * you want the form to appear on. i.e. array('myClassOne','myClassTwo')
      */
     public $showSearchForm = true;
 
@@ -119,8 +122,9 @@ abstract class ModelAdmin extends LeftAndMain
      * a subclass of {@link BulkLoader} (mostly CSV data).
      * By default {@link CsvBulkLoader} is used, assuming a standard mapping
      * of column names to {@link DataObject} properties/relations.
+     * Use {@link $managed_models} keys.
      *
-     * e.g. "BlogEntry" => "BlogEntryCsvBulkLoader"
+     * Example: "BlogEntry" => "BlogEntryCsvBulkLoader"
      *
      * @config
      * @var array
@@ -485,23 +489,25 @@ public function getManagedModels()
      * with a default {@link CsvBulkLoader} class. In this case the column names of the first row
      * in the CSV file are assumed to have direct mappings to properties on the object.
      *
-     * @return array Map of model class names to importer instances
+     * @return array Map of model keys to importer instances (same keys as $managed_models)
      */
     public function getModelImporters()
     {
-        $importerClasses = $this->config()->get('model_importers');
+        $importers = [];
+        $importerSpec = $this->config()->get('model_importers');
+        $models = $this->getManagedModels();
 
-        // fallback to all defined models if not explicitly defined
-        if (is_null($importerClasses)) {
-            $models = $this->getManagedModels();
-            foreach ($models as $modelName => $options) {
-                $importerClasses[$modelName] = 'SilverStripe\\Dev\\CsvBulkLoader';
+        foreach ($models as $modelName => $options) {
+            $modelClass = $options['dataClass'];
+            if (isset($importerSpec[$modelName])) {
+                $importerClass = $importerSpec[$modelName];
+            } elseif (isset($importerSpec[$modelClass])) {
+                $importerClass = $importerSpec[$modelClass];
+            } else {
+                $importerClass = CsvBulkLoader::class;
             }
-        }
-
-        $importers = array();
-        foreach ($importerClasses as $modelClass => $importerClass) {
-            $importers[$modelClass] = new $importerClass($modelClass);
+            // Needs to be indexed by name to avoid collisions
+            $importers[$modelName] = new $importerClass($modelClass);
         }
 
         return $importers;
@@ -601,14 +607,14 @@ public function ImportForm()
     public function import($data, $form, $request)
     {
         if (!$this->showImportForm || (is_array($this->showImportForm)
-                && !in_array($this->modelClass, $this->showImportForm))
+                && !in_array($this->modelTab, $this->showImportForm))
         ) {
             return false;
         }
 
         $importers = $this->getModelImporters();
         /** @var BulkLoader $loader */
-        $loader = $importers[$this->modelClass];
+        $loader = $importers[$this->modelTab];
 
         // File wasn't properly uploaded, show a reminder to the user
         if (empty($_FILES['_CsvFile']['tmp_name']) ||

diff --git a/tests/php/ModelAdminTest.php b/tests/php/ModelAdminTest.php
@@ -2,17 +2,19 @@
 
 namespace SilverStripe\Admin\Tests;
 
-use SilverStripe\Admin\Tests\ModelAdminTest\Contact;
-use SilverStripe\Admin\Tests\ModelAdminTest\Player;
-use SilverStripe\Control\HTTPRequest;
+use SilverStripe\View\ArrayData;
 use SilverStripe\Control\Session;
+use SilverStripe\Dev\CsvBulkLoader;
+use SilverStripe\Dev\FunctionalTest;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Security\Permission;
 use SilverStripe\Forms\GridField\GridField;
+use SilverStripe\Admin\Tests\ModelAdminTest\Player;
+use SilverStripe\Admin\Tests\ModelAdminTest\Contact;
+use SilverStripe\Forms\GridField\GridFieldPrintButton;
 use SilverStripe\Forms\GridField\GridFieldExportButton;
 use SilverStripe\Forms\GridField\GridFieldImportButton;
-use SilverStripe\Forms\GridField\GridFieldPrintButton;
-use SilverStripe\Security\Permission;
-use SilverStripe\Dev\FunctionalTest;
-use SilverStripe\View\ArrayData;
+use SilverStripe\Admin\Tests\ModelAdminTest\ModelAdminTestBulkLoader;
 
 class ModelAdminTest extends FunctionalTest
 {
@@ -164,6 +166,46 @@ public function testGetManagedModels()
         );
     }
 
+    public function testModelImporters()
+    {
+        $admin = new ModelAdminTest\MultiModelAdmin();
+
+        $importers = $admin->getModelImporters();
+
+        $this->assertArrayHasKey(
+            Contact::class,
+            $importers,
+            'Infers importer for non-listed models'
+        );
+        $this->assertInstanceOf(
+            CsvBulkLoader::class,
+            $importers[Contact::class],
+            'Sets default importer for non-listed models'
+        );
+
+        $this->assertArrayHasKey(
+            Player::class,
+            $importers,
+            'Discovers models referenced by class'
+        );
+        $this->assertInstanceOf(
+            ModelAdminTestBulkLoader::class,
+            $importers[Player::class],
+            'Sets explicitly defined importer for models referenced by class'
+        );
+
+        $this->assertArrayHasKey(
+            'Player',
+            $importers,
+            'Discovers models referenced by alias'
+        );
+        $this->assertInstanceOf(
+            ModelAdminTestBulkLoader::class,
+            $importers['Player'],
+            'Sets explicitly defined importer for models referenced by alias'
+        );
+    }
+
     public function testGetManagedModelTabs()
     {
         $mock = $this->getMockBuilder(ModelAdminTest\MultiModelAdmin::class)

diff --git a/tests/php/ModelAdminTest/ModelAdminTestBulkLoader.php b/tests/php/ModelAdminTest/ModelAdminTestBulkLoader.php
@@ -0,0 +1,10 @@
+<?php
+
+namespace SilverStripe\Admin\Tests\ModelAdminTest;
+
+use SilverStripe\Dev\CsvBulkLoader;
+
+class ModelAdminTestBulkLoader extends CsvBulkLoader
+{
+
+}
diff --git a/tests/php/ModelAdminTest/MultiModelAdmin.php b/tests/php/ModelAdminTest/MultiModelAdmin.php
@@ -2,9 +2,10 @@
 
 namespace SilverStripe\Admin\Tests\ModelAdminTest;
 
+use SilverStripe\Dev\TestOnly;
 use SilverStripe\Admin\ModelAdmin;
 use SilverStripe\Control\Controller;
-use SilverStripe\Dev\TestOnly;
+use SilverStripe\Admin\Tests\ModelAdminTest\ModelAdminTestBulkLoader;
 
 class MultiModelAdmin extends ModelAdmin implements TestOnly
 {
@@ -21,6 +22,13 @@ class MultiModelAdmin extends ModelAdmin implements TestOnly
         ]
     ];
 
+    private static $model_importers = [
+        // Infer Contact importer
+        // Contact::class,
+        'Player' => ModelAdminTestBulkLoader::class,
+        Player::class => ModelAdminTestBulkLoader::class,
+    ];
+
     public function Link($action = null)
     {
         if (!$action) {