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.
Comment | File | Size | Author |
---|---|---|---|
#38 | interdiff-2388353-36-38.txt | 265 bytes | BR0kEN |
#38 | serial-custom-step-value-2388353-38.patch | 6.88 KB | BR0kEN |
Comments
Comment #1
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #2
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #3
ikeigenwijs CreditAttribution: ikeigenwijs commentedWorks 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.
Comment #4
ikeigenwijs CreditAttribution: ikeigenwijs commenteda remark about coding standards fixed, got them all now i think.
Comment #5
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #6
ikeigenwijs CreditAttribution: ikeigenwijs commentedWhen 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.
Comment #7
ikeigenwijs CreditAttribution: ikeigenwijs commentedBetter help text and small bug fix.
Runs in production.
We have an auto increment of 10.
Comment #8
cpeeters CreditAttribution: cpeeters commentedI 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!
Comment #9
ikeigenwijs CreditAttribution: ikeigenwijs commentedthx cpeeters i owe you one ;-)
Comment #10
colanThanks 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.
Fix the spelling mistake and the extra space in the comment, and make the code line cleaner, like so: "$n = (int) ($i / $m);"
Didn't you already cast this in the previous line?
Remove these two extra blank lines.
Wrong indenting here.
Spelling, punctuation, and spacing need work here.
Leave a blank line between these, and end with a ".".
The spacing is off, and there's missing punctuation.
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. ;)
Comment #11
colanNew features must apply to HEAD.
Comment #12
rpayanmPlease review.
Comment #13
rpayanmWrong tabs.
Comment #14
ikeigenwijs CreditAttribution: ikeigenwijs commented@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.
Comment #15
cpeeters CreditAttribution: cpeeters commentedModule works fine, just like before the extra improvements.
I did same tests as in comment #8. My remarks were incorporated.
Comment #16
colanComment #17
ikeigenwijs CreditAttribution: ikeigenwijs commentedColan, 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.
Comment #18
colanCan 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!
Comment #19
Wim Leers"but first device with"
→ that doesn't make sense :)
Also: violates the 80 cols rule.
The ASCII art looks broken because there aren't enough spaces after "pages".
s/mysql/MySQL/
s/localy/locally/
s/mysql related/MySQL-related/
What is "the correction factor"?
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.This is placeholder text. I cannot imagine this is the intended help text.
Eh…? This doesn't belong here.
None of these belong here.
Drupal uses U.S. English, not British English. Hence "neutralize" is what you want to use.
This English is plain broken.
Whitespace.
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.
Comment #20
ikeigenwijs CreditAttribution: ikeigenwijs commentedI 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.
Comment #21
Wim LeersPlease 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
Comment #22
ikeigenwijs CreditAttribution: ikeigenwijs commentedok 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.
Comment #23
Wim Leers80 cols.
ASCII art still broken.
These comments only add noise.
Unnecessary blank line here.
The parentheses around the second check are unnecessary, and strict equality can be used there.
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.
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.
Comment #24
ikeigenwijs CreditAttribution: ikeigenwijs commentedSorry 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 patchesComment #25
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #26
Wim LeersI'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.)
This is removed…?
Nope, it's now over here.
Comment #27
ikeigenwijs CreditAttribution: ikeigenwijs commentedStarted 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.
Comment #28
cpeeters CreditAttribution: cpeeters commentedPatch still functions like it should!
Comment #29
colanNeeds spaces around the "=".
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?
Missing space.
Would probably be best to make sure this isn't negative either.
Comment #30
ikeigenwijs CreditAttribution: ikeigenwijs commentedRemark 1: fixed
Remark2:
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.
Comment #31
ikeigenwijs CreditAttribution: ikeigenwijs commentedHit save once and double posted.
Comment #32
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #33
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #34
BR0kENGood 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.
Comment #35
ikeigenwijs CreditAttribution: ikeigenwijs commentedOn field by field basis would indead be more flexible.
Comment #36
BR0kENPlease look on my implementation and test it.
Comment #37
colanThe module packaging info definitely shouldn't be in there.
Comment #38
BR0kENAhh, my bad.
Comment #39
BR0kENBy the way, we should enable the automated testing for this module. TestBot will be able to applying patches and run the SimpleTest automatically.
Comment #40
ikeigenwijs CreditAttribution: ikeigenwijs commentedI 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.
Comment #41
BR0kENI'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.
Comment #42
colanDone!
Comment #43
ikeigenwijs CreditAttribution: ikeigenwijs commentedIt 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
If you forget 1 serial field custom code will go haywire
Comment #44
ikeigenwijs CreditAttribution: ikeigenwijs commentedShould be doubles and
Allow negatives as well.
More functionality for the same price
Comment #45
m1r1k CreditAttribution: m1r1k at Propeople (now part of FFW) for Propeople (now part of FFW) commentedHi @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.
Comment #46
ikeigenwijs CreditAttribution: ikeigenwijs commentedHi @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
Comment #47
BR0kENYou 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.
Comment #48
ikeigenwijs CreditAttribution: ikeigenwijs commented@ 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