Use case:

When the db server is spread out over multiple instances the auto_increment_increment setting is generally larger than one to avoid conflicts between the different servers of the same db instance.
The problem is that is you use serial the numbers go 10 fold faster and is confusing for users.

Example:
auto increment start value: server number ->2
auto increment increment step value: 10

give following serial numbers:
2,12,22,32,...,92,102

Proposed solution:
Settings page with 1 textbox to set the auto_increment_increment value set in mysql
default value 1
or
Settings page with 1 textbox where the auto_increment_increment value set in mysql is retrieved
And a check box to set the correction factor.

The Value returned by serial field should be corrected like this:

serial= (int)((get next auto increment from db) / value_set_at_serial_settings_page +1)

Unfortunately the auto increment is differently implemented for different DB's
http://stackoverflow.com/questions/14897774/change-auto-increment-attrib...

I would focus for now on mysql.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ikeigenwijs’s picture

Title: Extra settings page to cancel the effect of the auto increment value of the db setting. » Extra settings page to cancel the effect of the auto increment step value of the db setting in case of somthing different of 1.
Issue summary: View changes
ikeigenwijs’s picture

Assigned: Unassigned » ikeigenwijs
ikeigenwijs’s picture

Status: Active » Needs review
FileSize
4.73 KB

Works on my local machine.

Can also be used to change the increment value for the whole site for serial field.
An addition would be to be able to put it on an a per field basis.
Maybe in the field widget.

Usage is not without its risks, mentioned that on the help page.
patched against the d7 1.3 version.

ikeigenwijs’s picture

Issue tags: +Novice
FileSize
5.49 KB

a remark about coding standards fixed, got them all now i think.

ikeigenwijs’s picture

Assigned: ikeigenwijs » Unassigned
ikeigenwijs’s picture

When testing with simplytest.me the patch does not take, i dont know why.
The patch works locally on clean install, reviewed by Drugo.
You need to modify the db setting for the auto increment value for testing this patch with want to neutralise it.

ikeigenwijs’s picture

Better help text and small bug fix.
Runs in production.
We have an auto increment of 10.

cpeeters’s picture

Status: Needs review » Reviewed & tested by the community

I tested the following thing (on request):

Db auto increment step value is by default 1:
Applying the patch with no settings changed does not change the existing fields.
New fields still behave as expected.
New content type with existing field and a new serial field still works as expected

Changed the db auto increment step value to 7.
But did not change the setting on the settings page of serial_field.
Same behavior as before. 0,7,14,21,28,....

Changed the setting on the settings page of serial_field to 7 and thus neutralizing the 7 increment step value.
Works as advertised, the serial fields are again sequential.

Thus, in summary:
Scenario 1: for existing sites/databases: this does not change the existing fields and functionality!

Scenario 2: for new projects: functionality is improved (changing the setting to anything other than 1 in correlation with db increment step value)

Scenario 3: special case in which patch is applied to existing database and serial_field is changed to anything other than 1.
For example: existing db has step size of 5, and the following numbers are already present: 0,5,10,15,20 and 25
If configuration of serial_field is 1 (default), next number would be 30
If configuration of serial_field is changed to 5, next number would be 6 (30/5=6). 6 is lower than last assigned number (25), thus resulting in possible overlap with existing entries with numbers 10, 15, 20 and 25, rendering these numbers non-unique!
However, this can be solved following these instructions: when changing the serial_field configuration, multiply the next auto increment value in the db with your configuration number (you only have to do this once for every serial field table). In the example, you would have to set the configuration of serial_field to 5 and modify the next auto increment value from 30 to 30*5 =>150 in the db. This would result in the next number being 30 (which is correct) and then followed by 31,32,33,...

So good to go!

ikeigenwijs’s picture

thx cpeeters i owe you one ;-)

colan’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for contributing!

I'd like to get some other thoughts on this approach (in other words, more reviews), but these items would definitely need to be fixed before a commit. Please read the coding standards.

+++ b/serial.inc
@@ -165,8 +165,10 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
+  // Return the new unique serial value but first devide  with the correction factor.
+  $sid= (int)$sid / variable_get('serial_increment', 1);

Fix the spelling mistake and the extra space in the comment, and make the code line cleaner, like so: "$n = (int) ($i / $m);"

+++ b/serial.inc
@@ -165,8 +165,10 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
+  return (int)$sid;

Didn't you already cast this in the previous line?

+++ b/serial.install
@@ -5,6 +5,8 @@
+
+

Remove these two extra blank lines.

+++ b/serial.module
@@ -5,6 +5,90 @@
+// Main module help for the serial module
...
+// Help for another path in the block module

Wrong indenting here.

+++ b/serial.module
@@ -5,6 +5,90 @@
+      return '<p>' . t('The serial field relies on the mysql database auto increment value, to provide unique serial fields(asyncronus writing safe)</br>
+                        On shared hosting or shared databases the auto_increment_increment is not editable, but lucaly on the settings page it is.</br>
+                        The mysql related <a href="@mysql_help"> help</a> page.</br>
...
+                        <li>First number will be 0 when using the correction factor</li>
+                        <li>The used formula is: sid= sid_fromdb / correction_factor</br>
+                        eg: when auto increment is 7 you put 7 in the field</li>',

Spelling, punctuation, and spacing need work here.

+++ b/serial.module
@@ -5,6 +5,90 @@
+ * Implements hook_menu().
+ * That calls the settings form

Leave a blank line between these, and end with a ".".

+++ b/serial.module
@@ -5,6 +5,90 @@
+    '#title' => t('Increment neutralise factor  to use'),
...
+    '#description' => t("Set the wanted value you want to use neutralize the increment. Can also be used to change the increment value otherwise<br/>
+						    The used formula is: sid= sid_fromdb / correction_factor</br>
+                            eg: when auto increment is 7 you put 7 in the field"),

The spacing is off, and there's missing punctuation.

+++ b/serial.module
@@ -5,6 +5,90 @@
+  $increment = $form_state['values']['serial_increment'];
+  if (!is_numeric($increment)) {
+    form_set_error('serial_increment', t('You must enter an integer for the increment field.'));
+  }
+
+  if (is_numeric($increment) && $increment==0) {
+    form_set_error('serial_increment', t('You must enter an integer other than 0'));
+  }

I think we can combine this into one if condition "!is_numeric($increment) || ($increment == 0)". English would be something like, "You must enter a non-zero integer." Don't forget the punctuation. ;)

colan’s picture

Version: 7.x-1.3 » 7.x-1.x-dev

New features must apply to HEAD.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
3.75 KB

Please review.

rpayanm’s picture

FileSize
3.76 KB

Wrong tabs.

ikeigenwijs’s picture

FileSize
4.99 KB

@colan: thx for the extensive answer
@rpayanm: thx for the quick replay, we meet again ;-)

Senario1: old functionality for existing sites

Patch works on simplytest.me with default settings of 1.
Could not change the db increment step there so did it localy.

Senario2: neutralizing the increment step
Also works.

I checked for the remarks of colan in the patch, all seems to be addressed, but the remark about wrong indentation i do not know what to do, I indented the help text according to html structure.
I mode some small changes, extra help text for the special case and a link to the help under the configuration field.
so more review

@colan: it took me 2 weeks on irc to find someone to review the patch.

cpeeters’s picture

Status: Needs review » Reviewed & tested by the community

Module works fine, just like before the extra improvements.

I did same tests as in comment #8. My remarks were incorporated.

colan’s picture

Title: Extra settings page to cancel the effect of the auto increment step value of the db setting in case of somthing different of 1. » Allow for step value other than 1
ikeigenwijs’s picture

Colan, can this be commited to dev?

I ll work on the other related issues, otherwise its going to be complicated.
This one gives a settings page framework.

colan’s picture

Status: Reviewed & tested by the community » Needs review

Can you find another experienced Drupalist to review the code? I don't really have time to review right now. I took a quick look, but I'd appreciate another pair of eyes.

If you're opening other issues, feel free to put the settings page framework in those. After the first gets committed, we can remove it from the other patches.

Sorry about the trouble, but I'd like to keep this module very stable. Thanks!

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/serial.inc
    @@ -165,6 +165,9 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
    +  // Return the new unique serial value but first device with the correction factor.
    

    "but first device with"
    → that doesn't make sense :)

    Also: violates the 80 cols rule.

  2. +++ b/serial.module
    @@ -6,6 +6,91 @@
     //==================//
    +// Viewable pages //
    +//==================//
    

    The ASCII art looks broken because there aren't enough spaces after "pages".

  3. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +      return '<p>' . t('The serial field relies on the mysql database auto increment value, to provide unique serial fields (asyncronus writing safe).</br>
    

    s/mysql/MySQL/

  4. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +                        On shared hosting or shared databases the auto_increment_increment is not editable, but localy on the settings page it is.</br>
    

    s/localy/locally/

  5. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +                        The mysql related <a href="@mysql_help">help</a> page.</br>
    

    s/mysql related/MySQL-related/

  6. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +                          <li>First number will be 0 when using the correction factor.</li>
    

    What is "the correction factor"?

  7. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +                          <li>The used formula is: sid = sid_fromdb / correction_factor</br>
    +                          eg: when auto increment is 7 you put 7 in the field</li>
    +                          </br>
    

    This feels like information that doesn't belong in hook_help(), but belongs in the README. Or perhaps in the UI. But not here.

    Not just this part, but actually everything in this hook_help() implementation.

    hook_help() is intended for high-level help to get people started, to explain what a module is for. Not for details like here.

  8. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +    case 'admin/serial':
    +      return '<p>' . t('More help or same help text') . '</p>';
    

    This is placeholder text. I cannot imagine this is the intended help text.

  9. +++ b/serial.module
    @@ -6,6 +6,91 @@
    + * That calls the settings form
    

    Eh…? This doesn't belong here.

  10. +++ b/serial.module
    @@ -6,6 +6,91 @@
    + * For the settings form
    ...
    + * The value must be an integer
    

    None of these belong here.

  11. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +    '#title' => t('Increment neutralise factor to use'),
    

    Drupal uses U.S. English, not British English. Hence "neutralize" is what you want to use.

  12. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +    '#description' => t('Set the wanted value you want to use neutralize the increment. Can also be used to change the increment value otherwise.<br/>
    

    This English is plain broken.

  13. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +function serial_settings_form_validate($form, &$form_state) {
    +
    +  $increment = $form_state['values']['serial_increment'];
    

    Whitespace.

  14. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +  if (!is_numeric($increment) || ($increment == 0)) {
    

    The parentheses in the second part of the expression are useless.

    And since you already checked whether it's numeric or not, you can use strict equality.

ikeigenwijs’s picture

Status: Needs work » Needs review
FileSize
25.69 KB

I kept the info in the hook help, because its important to get started immediately and when changing the value with existing values in db.
When changing the value a person would not go back to the readme to see the implications.

I fixed all other remarks.

Wim Leers’s picture

Please provide an interdiff, that makes it much easier for reviewers to see what has changed relative to the previous patch you posted.

See https://www.drupal.org/documentation/git/interdiff

ikeigenwijs’s picture

FileSize
17.89 KB

ok got the interdiff i think.
I don' really see the benefit but enjoy.

I have to admit and anderstand the threshold now for contrebuting.
It getting tiresome to jump through all these hoops if you want to contribute and i am really motivated.

The patch is running unchanged on our production for 2 months now.new data being added at 1000 of serial fields a week.
Patch does not change existing behaviour on old installs, so this should be an quick fix.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/serial.inc
    @@ -165,6 +165,9 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
    +  // Return the new unique serial value but first device with the correction factor.
    

    80 cols.

  2. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +// Viewable pages //
    

    ASCII art still broken.

  3. +++ b/serial.module
    @@ -6,6 +6,91 @@
    + * That calls the settings form
    ...
    + * For the settings form
    ...
    + * The value must be an integer
    

    These comments only add noise.

  4. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +function serial_settings_form_validate($form, &$form_state) {
    +
    +  $increment = $form_state['values']['serial_increment'];
    

    Unnecessary blank line here.

  5. +++ b/serial.module
    @@ -6,6 +6,91 @@
    +  if (!is_numeric($increment) || ($increment == 0)) {
    

    The parentheses around the second check are unnecessary, and strict equality can be used there.

  6. … at this point I stopped, because I noticed that #20 and #22 still include pretty much all the problems I pointed out in #19.
  7. … and worse still, the patch originally was 4-5 KB. Now it's 25 KB. Because the coding style has been changed everywhere, including in existing code. Why?!? So the patch in #20 is actually a step back compared to the one in #14!

I don' really see the benefit but enjoy.

Isn't it obvious that reviewers don't want to have to read the entire patch again, but only see the new changes relative to the previous patch? Otherwise they manually have to compare patches to spot the differences.

I have to admit and anderstand the threshold now for contrebuting.
It getting tiresome to jump through all these hoops if you want to contribute and i am really motivated.

It is also getting tiresome to repeatedly point out the same problems in your patch.

You e-mailed me directly to get a review. I did that for you, even though you should have just traded reviews with somebody else. Countless typos and coding standards violations I had to encounter, which you could easily have prevented (using e.g. the coder module). It seems you want to use your own code style. If we all did that, then Drupal could would be impossible to read. It is precisely why we have coding standards in the first place: to make it easier to read each other's code, to ensure all code looks like it was written by the same person.

In other words: you're implying that your time is being wasted, but instead, you're actually wasting time of others, by not abiding the rules we have that exist precisely to make collaboration easier!
"ikeigenwijs" is Dutch for "I, stubborn" in English — is stubbornness the problem here?

I hope the above makes you see the problems with your patches, and will hopefully make you consider rerolling your patch in #14 to comply with the coding standards and have correct English.

ikeigenwijs’s picture

FileSize
14.48 KB
17.93 KB

Sorry Wim i was frustrated.
And patch 20 is just the wrong one, i dont know how that got up there( im doing this on 4 different environments )
So that explains a lot.

I did not know about coder module.
I used coder, i got no more warnings, but the php sniffer part is not working correctly i think.

The interdiff still gives a lot of change what i find weard.
I know your doing your best to keep code up to par.

That the coding style is changed for the old code is that i tried "porley apperently" to apply the coding standards i think or phpstorm did it automatic (i use the specific drupal settings with no changes).

I reviewed more than 10 or 20 patches in the mean time not on coding standards obviously but functionality, i already really begged on different fora and irc channels, so the review swapping really does not work for this small module. An one did but did not post here :-s

I also dont know why the patch first removes certain lines of code and than readdes them later, with no change what so ever, thats why the patch is so big.
for example hook serial_field_info.
i use diff master patch_branch > xx.patch to create the patches

ikeigenwijs’s picture

Wim Leers’s picture

I'm afraid the patch in #24 is still not acceptable. The problem is that all the existing code has been modified/indented. Either your IDE/text editor did, or you did, and you forgot, maybe?

In any case, your IDE/text editor is configured very much incorrectly, because there's 4-space indentations all over the place. I remember that the patch you worked on at the code sprint in Ghent last year had the same problem. I fixed all that for you back then ;)

Please just set up your IDE/text editor in accordance with the coding standards (2-space indentations), and disable its "auto-reformat code" feature, because *that* is what's causing all these problems probably.

(I hope you understand that that is not acceptable. It makes it *extremely* difficult to review your code, because rather than changing only the thing you're wanting to change, you're changing everything else too! If we'd commit patches like that, it'd be impossible to find where/when something was changed and why.)

  1. +++ b/serial.module
    @@ -5,116 +5,215 @@
    -//==================//
    -// Field Definition //
    -//==================//
    ...
    - * Implements hook_field_info().
    ...
    -function serial_field_info() {
    -  return array(
    -    'serial' => array(
    -      'label' => t('Serial'),
    -      'description' => t('Auto increment serial field type.'),
    -      'default_widget' => 'serial',
    -      'default_formatter' => 'serial_formatter_default',
    -    ),
    -  );
    

    This is removed…?

  2. +++ b/serial.module
    @@ -5,116 +5,215 @@
    +//==================//
    +// Field Definition //
    +//==================//
    +
    +  /**
    +   * Implements hook_field_info().
    +   */
    +  function serial_field_info() {
    +    return array(
    +        'serial' => array(
    +            'label' => t('Serial'),
    +            'description' => t('Auto increment serial field type.'),
    +            'default_widget' => 'serial',
    +            'default_formatter' => 'serial_formatter_default',
    +        ),
         );
    +  }
    

    Nope, it's now over here.

ikeigenwijs’s picture

Status: Needs work » Needs review
FileSize
5.2 KB

Started from scratch.
There was also something wrong with the command i used to create the patches.
This looks more like it i hope, its small again and a lot more readable.

I also ran the code check module, got a lot of style errors, but i only changed the rows i touched according to instructions and coding guidelines.

cpeeters’s picture

Patch still functions like it should!

colan’s picture

Status: Needs review » Needs work
+++ b/serial.inc
@@ -165,6 +165,9 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
+  $configured_factor=variable_get('serial_increment', 1);

Needs spaces around the "=".

+++ b/serial.module
@@ -5,6 +5,102 @@
+                      <li>The used formula is:
+                       sid = sid_fromdb / correction_factor</br>

Either I'm misunderstanding this, or it's wrong.

If the sid_fromdb is 6 and the correction_factor is 3, the sid will be 2, which is lower than 6. It needs to be greater than 6, correct?

+++ b/serial.module
@@ -5,6 +5,102 @@
+                          during this process(maintenance mode).</br>

Missing space.

+++ b/serial.module
@@ -5,6 +5,102 @@
+  if (!is_numeric($increment) || $increment == 0) {
+    form_set_error('serial_increment', t('You must enter a non-zero integer.'));
+  }

Would probably be best to make sure this isn't negative either.

ikeigenwijs’s picture

FileSize
5.2 KB
291 bytes

Remark 1: fixed

Remark2:

+++ b/serial.module

@@ -5,6 +5,102 @@
+                      <li>The used formula is:
+                       sid = sid_fromdb / correction_factor</br>

Either I'm misunderstanding this, or it's wrong.

You misunderstand,
Example:
auto increment start value: server number ->2
auto increment increment step value: 10

Will give the following serial numbers: (with the basic serial module)
2,12,22,32,...,92,102

With the patch:
Setting of correction factor of 10
Will give the following serial numbers: (with serial module + this patch)
0,1,2,3,...,9,10

There is a message explaining the impact of changing the correction factor on an existing project and a how to.
I want to stress that the default value of 1 does not change the default behavior so updating existing projects is no problem.

Remark 3: i think/hope you want a space in front of the ( otherwise no idea

Remark 4: I would not limit to positives only.
Negatives does not break the module in any way, so i would leave as open as possible.
There are certainly use cases where you want a negative unique incremented value.
But its your choice.

ikeigenwijs’s picture

Hit save once and double posted.

ikeigenwijs’s picture

Status: Needs work » Needs review
ikeigenwijs’s picture

Status: Needs work » Needs review
BR0kEN’s picture

Assigned: Unassigned » BR0kEN
Status: Needs review » Needs work

Good idea to implement a custom value for step, but no so good as it should be :)

We definitely should not use the global value as step value for each field in our system. I'll add the implementation to field settings when will have a free time.

ikeigenwijs’s picture

On field by field basis would indead be more flexible.

BR0kEN’s picture

Assigned: BR0kEN » Unassigned
Status: Needs work » Needs review
FileSize
7.2 KB

Please look on my implementation and test it.

colan’s picture

Status: Needs review » Needs work
+++ b/serial.info
@@ -5,3 +5,10 @@ core = 7.x
+; Information added by drupal.org packaging script on 2013-10-15
+version = "7.x-1.3"
+core = "7.x"
+project = "serial"
+datestamp = "1381844527"

The module packaging info definitely shouldn't be in there.

BR0kEN’s picture

Status: Needs work » Needs review
FileSize
6.88 KB
265 bytes

Ahh, my bad.

BR0kEN’s picture

By the way, we should enable the automated testing for this module. TestBot will be able to applying patches and run the SimpleTest automatically.

ikeigenwijs’s picture

I will test this Wednesday.
I have no experience with implementing the auto tests if you want to walk me through it want to help add it.

BR0kEN’s picture

Assigned: Unassigned » kirsh

I'm not talking about writing the auto test for module (anyway yet) and just want that some permitted user, @kirsh, for example, enable the auto testing mode for this module here.

colan’s picture

Done!

ikeigenwijs’s picture

Status: Needs review » Needs work

It does not work for our use case.
We need to neutralize a server wide auto increment of 10

In your code our configured factor needs to be 0.1 but is not allowed only integers.

In our case when the update goes throug the following steps have to be done

  • Site in maintenance mode
  • Update serial module
  • set the custom step value for all serial fields -->listing of all the fields /admin/structure/fields
  • Set site Live

If you forget 1 serial field custom code will go haywire

ikeigenwijs’s picture

+++ b/serial.module
@@ -174,13 +186,55 @@ function serial_field_widget_info() {
+        '#element_validate' => array('element_validate_integer_positive'),

Should be doubles and
Allow negatives as well.
More functionality for the same price

m1r1k’s picture

Status: Needs work » Needs review

Hi @ikeigenwijs,

Could you please provide life examples of using doubles and negative increment steps?

Also price is not the same unfortunately, because right now we are using `serial` field type, that is unsigned and integer number,
so to support your case we will have to switch to another field type, that is pretty complex one.

ikeigenwijs’s picture

Hi @m1r1k,

I don mean that the resulting number should by anything else than an integer.
I just want to be able to multiply (new patch) by -0.1 for example.
That would give in our case -1,-2,-3,-4 as numbers
If in the db the field is unsigned this is not possible

For our use case (see initial post) positives alone is sufficient and a global setting is also sufficient
But we need to divide by 10 or multiply by 0.1 to achieve the following numbers
1,2,3,4,5

BR0kEN’s picture

You want possibilities that shouldn't be in the module. You able to implement all that you want in your custom code.

Patch gives the possibility to change step value and, if it receive the RTBC status, will be applied. Otherwise this topic will be closed after two weeks.

ikeigenwijs’s picture

@ BRoken
Can you explain why this would not belong in this module?
And why its not ok to multiply with a double. The resulting nr is always an unique (unsigned) integer because of the cast to int. Except when multiplying with 0.

eg:
An user with auto increment of 10 multiply with 1,15 or 0,33 or 0,1548 still gives unique integers, not sensible ones i agree but its the responsibility of the user to choose the right config value.
The module is not broken in any way.
I tested various values.

The risk that an user uses an sensible value is very small.

Serial uses the auto increment value of the server to guaranty uniqueness.
Everybody that has a different auto increment value witch is a global server setting than 1 can never achieve
a serial field that counts upwards 1, 2 3 without gaps.
Serial per definition should allow to achieve 1,2,3,4 in any situation.

our auto increment value is 10.
So in default config we get 10,20,30 and with the current patch are unable to achieve 1,2,3,4