Merge pull request tripal#1929 from tripal/tv4g1-issue1928-long-prope…

…rty-names

Resolve discovered field name length problems

tripal/src/TripalField/TripalFieldItemBase.php

@@ -6,6 +6,7 @@
 use Drupal\tripal\TripalField\Interfaces\TripalFieldItemInterface;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
+use Drupal\field\Entity\FieldStorageConfig;
 use Drupal\tripal\TripalStorage\IntStoragePropertyType;
 use Drupal\tripal\TripalStorage\VarCharStoragePropertyType;
 use Drupal\tripal\TripalStorage\TextStoragePropertyType;
@@ -663,19 +664,35 @@ public static function discover(TripalEntityType $bundle, string $field_id, arra
    * an appropriate field name. It removes will always include the bundle
    * type as the prefix, followed by an underscore, followed by extra text
    * provided. It ensures that only alphanumeric values are present in the
-   * name and that it doesn't exceed 50 characters.
+   * name and that it doesn't exceed Drupal's maximum length.
    *
    * @param \Drupal\tripal\Entity\TripalEntityType TripalEntityType $bundle
    *   The TripalEntityType object with information about the bundle.
    * @param string $extra
    *   Extra text to add to the field name after the bundle name.
+   * @param int $cvterm_id
+   *   The cvterm_id value, only used when we need to truncate a long name.
+   *   You may leave this set to the default of zero if the cvterm_id is
+   *   not readily available, in which case a random unique ID is used,
+   *   however we recommend using the actual cvterm_id if it is available
+   *   so that the field name generated is predictable and reproducible.
+   *
    * @return string
    *   The generated field name.
    */
-  public static function generateFieldName(TripalEntityType $bundle, string $extra) {
+  public static function generateFieldName(TripalEntityType $bundle, string $extra, int $cvterm_id = 0) {
+    $max_length = FieldStorageConfig::NAME_MAX_LENGTH;
     $field_name = strtolower($bundle->getID() . '_' . $extra);
-    $field_name = preg_replace('/[^\w]/', '_', $field_name);
-    $field_name = substr($field_name, 0, 50);
+    $field_name = preg_replace('/[^\w]/u', '_', $field_name);
+    // If name is longer than Drupal allows, truncate and include the
+    // cvterm_id as a suffix to guarantee uniqueness.
+    if (mb_strlen($field_name) > $max_length) {
+      if (!$cvterm_id) {
+        $cvterm_id = uniqid();
+      }
+      $truncate_to = $max_length - strlen($cvterm_id) - 1;
+      $field_name = mb_substr($field_name, 0, $truncate_to) . '_' . $cvterm_id;
+    }
     return $field_name;
   }
 }

tripal/tests/modules/tripal_test/src/Plugin/Field/FieldType/TripalTestTextTypeItem.php

@@ -44,12 +44,8 @@ public static function tripalTypes($field_definition) {
    */
   public static function discover(TripalEntityType $bundle, string $field_id, array $field_definitions) : array {
 
-    // Initialize with an empty field list.
-    $field_list = [];
-
-    // Create a valid field.
-    $field_list[] = [
-      'name' => self::generateFieldName($bundle, 'test_field'),
+    $base_field = [
+      'name' => self::generateFieldName($bundle, 'test_field', 0),
       'content_type' => $bundle->getID(),
       'label' => 'Test',
       'type' => self::$id,
@@ -82,40 +78,30 @@ public static function discover(TripalEntityType $bundle, string $field_id, arra
       ],
     ];
 
+    // Initialize with an empty field list.
+    $field_list = [];
+
+    // Create a valid field.
+    $field_list[] = $base_field;
+
+    // The same field but with a long name including spaces and unicode that
+    // will be truncated to 32 characters: 'organism__test_field_but_with__1'
+    // cvterm_id is passed and should appear at the end of the field name
+    $field_2 = $base_field;
+    $field_2['name'] = self::generateFieldName($bundle, '🙈test field_but with_a very_very long_name', 1);
+    $field_list[] = $field_2;
+
+    // The same except cvterm_id is not passed, a random unique id should be appended
+    $field_3 = $base_field;
+    $field_3['name'] = self::generateFieldName($bundle, '🙈test field_but with_a very_very long_name');
+    $field_list[] = $field_3;
+
     // Create an invalid field.
-    $field_list[] = [
-      'name' => self::generateFieldName($bundle, 'test_field2'),
-      'content_type' => $bundle->getID(),
-      'label' => 'Test',
-      'type' => 'this_type_does_not_exist',
-      'description' => 'A test field',
-      'cardinality' => 1,
-      'required' => TRUE,
-      'storage_settings' => [
-        'storage_plugin_id' => 'drupal_sql_storage',
-        'storage_plugin_settings' => [],
-        'max_length' => 255,
-      ],
-      'settings' => [
-        'termIdSpace' => 'OBI',
-        'termAccession' => '0100026'
-      ],
-      'display' => [
-        'view' => [
-          'default' => [
-            'region' => 'content',
-            'label' => 'above',
-            'weight' => 10,
-          ],
-        ],
-        'form' => [
-          'default' => [
-            'region' => 'content',
-            'weight' => 10
-          ],
-        ],
-      ],
-    ];
+    $field_4 = $base_field;
+    $field_4['name'] = self::generateFieldName($bundle, 'test_field4', 0);
+    $field_4['type'] = 'this_type_does_not_exist';
+    $field_list[] = $field_4;
+
     return $field_list;
   }
 }

tripal/tests/src/Functional/Services/TripalFieldCollectionTest.php

@@ -273,14 +273,27 @@ public function testTripalFieldCollection() {
     // those fields as expected.
 
     // Make sure we see a valid field
-    //print_r($discovered_fields);
     $this->assertTrue(array_key_exists('organism_test_field', $discovered_fields['new']), "Missing the 'organism_test_field' key in the 'new' array returned by the discover() function.");
 
+    // Make sure we see a properly truncated field name with
+    // spaces and unicode character replaced with underscores
+    $this->assertTrue(array_key_exists('organism__test_field_but_with__1', $discovered_fields['new']), "Missing the 'organism__test_field_but_with__1' key in the 'new' array returned by the discover() function.");
+
+    // Same as above, but cvterm_id was not passed. The field name should
+    // end with a unique id
+    $found = FALSE;
+    foreach ($discovered_fields['new'] as $field) {
+      if (preg_match('/^organism__test_fie_[0-9a-f]{13}$/', $field['name'])) {
+        $found = TRUE;
+      }
+    }
+    $this->assertTrue($found, "Missing the 'organism__test_fie_[0-9a-f]{13}' key in the 'new' array returned by the discover() function.");
+
     // Make sure we have an invalid field. We don't need to test every
     // case where a field may be invalid. That happens above. We just need to
     // make sure that if a field is invalid that it is listed in the invalid
     // section with the reason included.
-    $this->assertTrue(array_key_exists('organism_test_field2', $discovered_fields['invalid']), "The 'organism_test_field2' key should be in the 'invalid' array returned by the discover() function.");
+    $this->assertTrue(array_key_exists('organism_test_field4', $discovered_fields['invalid']), "The 'organism_test_field4' key should be in the 'invalid' array returned by the discover() function.");
 
     // Now add the valid field to the content type. and check to make sure
     // it is listed as `existing` afterwards when discover() is called again.

tripal_chado/src/Plugin/Field/FieldType/ChadoOrganismTypeDefault.php

@@ -312,7 +312,7 @@ public static function discover(TripalEntityType $bundle, string $field_id, arra
 
     // Create a field entry in the list.
     $field_list[] = [
-      'name' => self::generateFieldName($bundle, 'organism'),
+      'name' => self::generateFieldName($bundle, 'organism', 0),
       'content_type' => $bundle->getID(),
       'label' => 'Organism',
       'type' => self::$id,

tripal_chado/src/Plugin/Field/FieldType/ChadoPropertyTypeDefault.php

@@ -272,7 +272,7 @@ public static function discover(TripalEntityType $bundle, string $field_id, arra
     // Create a field entry for each property type.
     foreach ($results as $recprop) {
       $field_list[] = [
-        'name' => self::generateFieldName($bundle, $recprop->cvterm_name),
+        'name' => self::generateFieldName($bundle, $recprop->cvterm_name, $recprop->cvterm_id),
         'content_type' => $bundle->getID(),
         'label' => ucwords($recprop->cvterm_name),
         'type' => self::$id,