Closed (outdated)
Project:
Password Policy
Version:
6.x-1.x-dev
Component:
Code
Priority:
Minor
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
9 Jan 2016 at 05:07 UTC
Updated:
10 May 2016 at 07:54 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
aohrvetpv commentedComment #4
aohrvetpv commentedComment #5
aohrvetpv commentedNot sure it is correct to use
drupal_get_schema()instead ofdrupal_get_schema_unprocessed()in the new update function.Comment #7
aohrvetpv commentedComment #8
eelkeblokAlthough the patch has already been committed, it should be pointed out that referring to the current active schema from an update hook is actually risky business. You should consider the situation when you might make a change to the schema in the future and somebody is updating from before this update hook.
So, although it goes against our nature as programmers, you should actually copy the field spec from the schema into your update hook.
Comment #9
aohrvetpv commentedThanks. I will fix it, but why is it a problem to use the current active schema? My understanding was other modules might alter the schema using
hook_schema_alter(), so usingdrupal_get_schema()would get the altered schema. So it seems like passing the return value fromdrupal_get_schema()todb_change_field()is just resaving the current schema.Comment #11
aohrvetpv commentedFWIW,
drupal_get_schema_unprocessed()is recommended in a comment fordb_change_field():https://api.drupal.org/comment/58298#comment-58298
Comment #12
eelkeblokYou have a fair point about altering the schema, although IMHO that in itself is something that should be avoided as much as possible (this whole problem in itself is a good reason for that). There are two problems with using drupal_get_schema:
- You might get the cached version (i.e. without your changes);
- More importantly, the *current* schema might not always be what it is right now, and thus not what this update hook was intended to create and not what subsequent update hooks expect (you need to look at this not from the current pov, but from a future pov where several more schema changes have taken place).
The first point can be circumvented by using the unprocessed variant, but then you won't get any alterations. The second point remains.
Comment #13
aohrvetpv commentedI am having trouble seeing the second problem you mention. Could you possibly give an example of how it could be a problem?
TRUEcould be passed for$rebuildofdrupal_get_schema()to avoid getting a cached version.When
password_policy_update_7106()is run, the schema will be whatever it was when the administrator installed the module, plus any changes made by runningpassword_policy_update_710{0,1,2,3,4,5}().Comment #14
aohrvetpv commentedI see the problem now. I was very confused.
Comment #15
aohrvetpv commented#4 was broken too, not just bad practice. Apologies to anyone tracking the -dev branch.
If I understand correctly, we could use
drupal_get_schema_unprocessed(), but then we might have to change the update function later per schema changes. Easier to just duplicate the field specification in the update function.Thanks for explaining the proper practice, eelkeblok.
Comment #16
aohrvetpv commentedSome indentation was off by a space in last patch.
Comment #18
aohrvetpv commentedComment #19
eelkeblokI'm taking the liberty to close this as outdated (at least considering its previous status, Patch to be ported). I take it the D6 version is no longer supported.