Support migrating existing values

Was there a reason why it was set to needs work?

This looks to have fixed migrate issues for me so marking it as RTBC, thanks @singularo

adding a new field into a existing type with data throw this:

The website encountered an unexpected error. Please try again later.
ArgumentCountError: Too few arguments to function Drupal\serial\SerialSQLStorage::generateValueFromName(), 1 passed in /var/www/html/web/modules/contrib/serial/src/SerialSQLStorage.php on line 228 and at least 2 expected in Drupal\serial\SerialSQLStorage->generateValueFromName() (line 81 of modules/contrib/serial/src/SerialSQLStorage.php).
Drupal\serial\SerialSQLStorage->generateValueFromName('serial_53a82524717ed0f144162755709ed095') (Line: 228)
Drupal\serial\SerialSQLStorage->initOldEntries('node', 'serial_test', 'field_serial_test') (Line: 50)
serial_field_config_create(Object)
call_user_func_array('serial_field_config_create', Array) (Line: 403)

we need to update the initOldEntries function too...

Needed a re-roll to apply via composer patches.

Will this work with migrating from 7 to 8?

@Jed_BH I can't see why not it doesn't look D6 specific.

For some reason my values got imported, but for some reason the next new value starts from 38k something. Any thoughts on how to fix that?

Thank you @singularo, looks good! I've changed the variable naming a bit to indicate that it is an external value, then set this value as null explicitly in generateValueFromName() and removed the int type hinting in the methods signature. Even if php5 support is dropped, as we are not using type hinting for primitive values in other methods, it should be a module wide change.

Also, it would be good to have test coverage for migrations #3061273: Create migration unit tests, I will work on this during the week.

Applying the patch from #13 results in this error on saving an existing node with the Serial field:

The website encountered an unexpected error. Please try again later.Drupal\Core\Entity\EntityStorageException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'field_reference_value' at row 1: INSERT INTO {node__field_reference} (entity_id, revision_id, bundle, delta, langcode, field_reference_value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array
(
[:db_insert_placeholder_0] => 576
[:db_insert_placeholder_1] => 1008
[:db_insert_placeholder_2] => program
[:db_insert_placeholder_3] => 0
[:db_insert_placeholder_4] => en
[:db_insert_placeholder_5] => -1
)
in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 847 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Previously re-saving an entity with no value in the serial field set the serial to 1 (see #3058891: Do not return a default value if the serial id has not not been initialized on existing entities). With this patch it attempts to set it to -1 and fails as the field schema is for unsigned integers.

Possible solutions:

1. Maintain NULL values for old entities (may require field schema update?)
2. Update patch to mimic the old behaviour by setting the value to 1 in SerialItem::preSave() if SerialItem::getSerial() returns NULL and the current value is -1.
3. As above but switch to using 0 as a more sensible empty value, as it at least doesn't clash with the default start value?
4. Update the schema to allow negative integers

All but (2) above change the behaviour when re-saving uninitialised entities, but given the module is in alpha and #3058891: Do not return a default value if the serial id has not not been initialized on existing entities looks like a bug (multiple entities end up with the same serial in the default case) (1) or (3) look like better options?

I have been testing the current patch in a new project and can confirm it works well in the case where there are no uninitialised entities. Happy to update the patch with either approach.

I am not 100% sure what this patch is intended to do; but certainly doesn't do what i would have thought.

Starting point:
users 1 (uid = 1), 1000 to 1015 (test accounts)

I want to migrate users with id from 1004 to 1010, so i delete existing users from 1003-1015.

I import (using Feeds, not sure if this should matter, with simple plugin i created which basically just imports the value) test users (1004, to 1010).
- the first time it worked but serial ids were 2001 to 2007 - which makes no sense
- i deleted them and tried again.. this time 2 failed, and ids of the rest were the same (2xxx)
- i deleted again and re-tried.. and this time they all failed:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1011' for key 'PRIMARY': INSERT INTO {serial_eaafd7fb525fde97bdace1f0a513d383} (sid, uniqid) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 1011 [:db_insert_placeholder_1] => 5e5598c9180601.96844770 )

I manually tried to insert a new user; he gets ID 1000, which is a duplicate of an existing user.

very little of this was what i was hoping for :(

i see the serial table now and it has an entry for 1011, which i guess is why the import with that value fails.. but i do not have a user in my system with that value.

perhaps what i am trying to do is not what the intent of this patch is.. i think what i need is to add a serial field but set it to disabled (at which time it acts like a simple text field) and then after migration is done, enable the serial effect to start up at either the value set in the field config or after the last value migrated into the db.

looking at the values stored in the serial table compared to those in the field table and it certainly looks corrupted.. so i deleted all the values from the serial table except the serial id =1 for uid = 1. i also deleted all the entries from the field table except that 1 as well.

this time when i import user with (intended) serial id values 1005 to 1011, i get very odd results.

serial table: entries for 1010 and 1011 (id 1 is gone), and missing most of them
field table: 1, 2004 to 2010

so clearly wrong.. but possibly something to do with my Feeds import target (which really is only making the Serial field visible in the UI)?

Reroll patch against current dev (adf0e9c).

And here's the re-roll diff, which I find more confusing than helpful, but maybe it will help someone else.

Re-rolled against 2.0.x

I wonder how this issue will be affected by #3414661: Serial not generating on entity save in certain contexts. I am adding that as a related issue.

Looking at the patch in #23, I have a few comments.

1.
+++ b/src/SerialSQLStorage.php
@@ -73,7 +73,7 @@ class SerialSQLStorage implements ContainerInjectionInterface, SerialStorageInte
   /**
    * {@inheritdoc}
    */
-  public function generateValueFromName($storageName, $delete = TRUE) {
+  public function generateValueFromName($storageName, $externalValue = NULL, $delete = TRUE) {

Normally, when adding a new optional parameter, we add it after the existing parameters, for backwards compatibility (BC).

2. @@ -81,9 +81,17 @@ class SerialSQLStorage implements ContainerInjectionInterface, SerialStorageInte
...
+      if (isset($externalValue)) {
+        $sid = $connection->insert($storageName)
+          ->fields(['sid' => $externalValue, 'uniqid' => $uniqid])
+          ->execute();
+      }
+      else {
+        $sid = $connection->insert($storageName)
+          ->fields(['uniqid' => $uniqid])
+          ->execute();
+      }

This can be simplified. Something like this:

$values = [...];
if (...) {
  $values['sid'] = ...;
}
$sid = $connection->insert(...)
  ->fields($values)
  ->execute();