Implement Features hooks to export and import configurations

Committed to master. Marking needs port cause I'm going to try merging in #985374: Drupal 7 Port and might clobber this patch in the process.

I think I saved it all in conflict resolution.

That patch only allowed the privacy policy to be exported. For this ticket to be closed in my opinion, we need to put the loading part in here as well. When I export and create a new site, none of the privacy policies show up.

subscribe

You're right, we're not making use of hook_default_password_policy() in the loading process. Unfortunately it looks like this will need some work to refactor the loading operation into something more CTools-friendly.

Loading part is missing in 7.x Version, too: #1250296: Exported Password Policy does not import on install.

Other modules have the same problem: #1357588: follow-up for import (needs to be optionally available when ctools is enabled)

Can I recommend just changing the pid to a varchar instead of auto inc, and not add a new field. It might make using the ctool load function easier.

I do not see any reason why you need both a id and a name field.

subscribe

How does this actually work at the moment?

what do I have to do to get the "faux-exportable" functionality working. I put my password policy in a feature but now I can't figure out how to get it to appear on my staging site.

Even just making a policy, putting it in a feature on my site then clicking "delete" within the same site (should be revert?) doesn't bring back the settings that I saved.

I'm very confused as to how this is supposed to be functioning.

The export is only one part of the functionality. With ctools you only have to add an export part to the schema array in hook_schema. For this module the export works.

Know you have to implement the import part. I'm working on a patch for the apachesolr module (#1357588: follow-up for import (needs to be optionally available when ctools is enabled)). It's work in progress. As I said before I think it's the same problem here. Have a look at this and perhaps publish a first patch for this module.

I've been having a look into the load side of things.

Hoping that replacing the function that loads by id with one that loads by name and uses the ctools load function will work, provided that i turn the object returned by ctools into the array format that the rest of the pp module expects...

Had some limited success using the ctools_export_ui but nothing worst posting here as a patch yet.

The fundamental change is really to move loading to a machine name-based method. This is probably the fundamental step to integrating CTools.

yeah, that's basically what i thought, the function that saves the roles associated with each policy needs to be updated to store the machine name of the policy as well as just the pid too.

Maybe I am missing something but why doesn't this module use the Features API?

Because nobody has provided a patch for it yet.

I have work on a rewrite of password policy that does have exportable policies here
http://drupal.org/sandbox/e2thex/1380342
here is the dicusion about making it a 2.x version of password policy
http://drupal.org/node/1380766

#1380766: Proposed rewrite using ctools export and plugins

I don't think this is going anywhere, let's focus on full CTools exportables support for 7.x-1.x right now. If someone provides a patch, I'm happy to reopen this issue later.

#1575804: Use the CTools API to load objects

We need the ability to roll out PP configuration across ~ 26 sites and have started to tinker with this. It looks like the export functionality is semi-working although I'm inclined to think the 'export' schema array needs "primary key" (judging from other modules).

Other parts of the module (namely the policy listing page) needs to be tweaked to fetch configuration from ctools, not the database (if the object is overriden it will be in the database). Is my thinking right?

Willing to work on this for 7.x-1.x but would like some input.

Hi,
Although 7.x-2.x has a full ctools plugin aproach it would be great to have an approach to import in 7.x-1.x.
This patch provides an approach to import password_policy component exportables. It has been tested with drush fe, drush fu and drush fr.

Regards

Regards.

24: password_policy-features-985758-24.patch queued for re-testing.

Hi,
the patch is passing the test in my local environment.
I am no able to figure out why is not passing in drupal.org

Ok, passing now so ready to test...

Having done my own testing with this code, I would suggest it makes more sense if going down this route to write your own export code as well. As you are probably aware, although reverting will create the database entries, most of the time you can't revert because the export looks at the default values provided in code and thinks all is well, ignorant of the fact that password_policy only loads from the database.

As noted, it would be possible to re-code password_policy to use ctools load, but in actual fact it's much easier to write your own export. That will also allow you to properly incorporate roles, which are not handled in the above patch. I have some rough-and-ready code, not yet translated to a patch, which includes role settings as part of each policy rather than as a separate export. The revert hook then needs to be tweaked accordingly.

I have pasted in the relevant code below. The export format is based on what ctools generates. It doesn't really need to be so verbose but I started by trying to mimic what ctools does and haven't had time for a tidy-up yet.

<?php
/**
 * Implementation of hook_features_revert()
 */
function password_policy_features_revert($module) {

  // Retrieving default data.
  $default_policies = features_get_default('password_policy', $module);

  if (empty($default_policies)) {
    return;
  }

  // Retrieving db data.
  $data_base_policies = db_query("select pid, name from {password_policy}")->fetchAllKeyed(1, 0);

  foreach ($default_policies as $name => $default_policy) {

    $key = array();

    if (isset($data_base_policies[$name])) {
      $key = 'pid';
      $default_policy->pid = $data_base_policies[$name];
    }

    $record = (array)$default_policy;
    drupal_write_record("password_policy", $record, $key);    
    $roles = $default_policy->roles;
    $existing_roles = db_select('password_policy_role', 'ppr')
      ->fields('ppr', array('rid'))
      ->condition('pid', $record['pid'])
      ->execute()
      ->fetchAllAssoc('rid');
    db_delete('password_policy_role')
    ->condition('pid', $record['pid'])
    ->execute();
    foreach($roles as $rid => $role) {
      if(!array_key_exists($rid, $existing_roles)) {
        db_insert('password_policy_role')
        ->fields(array('pid' => $record['pid'], 'rid' => $rid))
        ->execute()
        ;
      }
    }

  }

}

function password_policy_features_export_render($module_name, $data, $export= null) {
 $code = array();
 $code[] = '$password_policy = array();';
 foreach ($data as $name) {
    $policy = db_select('password_policy', 'p')
        ->fields('p')
        ->condition('name', $name)
        ->execute()
        ->fetch();
    if($policy) {
      // get roles for this policy
      $roles = db_select('password_policy_role', 'ppr')
      ->fields('ppr', array('rid'))
      ->condition('pid', $policy->pid)
      ->execute()
      ->fetchAllAssoc('rid');
      $result = new stdClass();
      $result->disabled = (boolean)!$policy->enabled;
      $result->api_version = 1;
      $result->name = $policy->name;
      $result->description = (string)$policy->description;
      $result->enabled = (boolean)$policy->enabled;
      $result->roles = $roles;
      $result->policy = @unserialize($policy->policy);
      $result->created = $policy->created;
      $result->expiration = (int)$policy->expiration;
      $result->warning = (string)$policy->warning;
      $result->weight = $policy->weight? $policy->weight: FALSE;
      $code[] = "  \$password_policy['{$name}'] = " . ctools_var_export($result) .";";
    }
 }
 $code[] = "return \$password_policy;";
 $code = implode("\n", $code);
 return array('default_password_policy' => $code);  
}

?>

The codebase has changed from the last patch for features functionality. Attaching an updated version.

It looks like the former title of this issue got accomplished for 2.0 in #1380766: Proposed rewrite using ctools export and plugins.

Some of us still need this in the fully supported 1.x branch. So I'm attaching a patch that updates the one in #24 above, and adds minimal support for setting the policy's roles. This will almost certainly break if your feature contains more than one policy, or if you use exported roles form another site that do not have matching role IDs.

But it's good enough for us, and might be useful to many others, so I'm sharing.

+1 to get this feature working on 1.x branch, am testing out matt2000's patch now

After removing the old policies from features (wouldn't revert from an empty policy in the db) and re-exporting them I then changed a property of the policy and did a drush fr my_feature and got the following errors:

~/project/sites/all/modules/custom/features (master) $ drush fr moh_users_roles_and_permissions -y
WD php: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '2-2' for key 'PRIMARY': INSERT INTO {password_policy_role} (pid, rid) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1);   [error]
Array
(
    [:db_insert_placeholder_0] => 2
    [:db_insert_placeholder_1] => 2
)
 in drupal_write_record() (line 7202 of /home/adam/project/includes/common.inc).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '2-2' for key 'PRIMARY': INSERT INTO {password_policy_role} (pid, rid) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array
(
    [:db_insert_placeholder_0] => 2
    [:db_insert_placeholder_1] => 2
)
 in drupal_write_record() (line 7202 of /home/adam/project/includes/common.inc).
Drush command terminated abnormally due to an unrecoverable error.

Will look into it further later this afternoon.

Here's a patch with proper handling for duplicate entries.

Here's a not-so-clearly-broken patch... :-P

Hey, I've started working on a more robust patch myself that doesn't use try/catch, you've also included other changes to the validate function in that patch which don't apply to this issue. I'll post my progress later today but it fits in with your patch and extends it to check the database_policy_roles using a database query much like your password_policy query. I've got it to a point where it doesn't error and it correctly reverts an existing policy, I just want to make it so if the policy doesn't exist at all, it'll create it.

Cheers

Fix undefined index...

Ok, please read my comments in #42, I don't think that using a try/catch and checking on an arbitrary error code is the best way to go about this (especially since I'm running postgres). Here's my patch that correctly reverts an existing policy, it will not create the policy if it doesn't exist in the database at all (I'm not sure why and don't have the time to look into it). You've also still included the changes to the validate function.

Patch attached, based of #35.

EDIT: The policy does get correctly imported upon enabling a new feature containing the policy :)

Removing the @ symbol in front of the drupal_write_record

I applied #45, but unfortunately it did nothing for the feature I already created with an exported password policy (it had previously removed the policy upon reverting, but I would expect that when this code is complete, it would also detect missing policies (which is probably a fairly common scenario, where new policies may be introdiced in subsequent versions of the same feature).

In fact, I am inclined to call the fact that there is only an export and not an import simply a bug. Without knowing the details of this issue, one might go into Features, find this module allows exporting policies in a Feature, do that, and then spend time figuring out why it isn't working on a new environment (in fact, that is exactly how I came upon this issue). I think the most simple way to fix this bug, for the time being, is to simply remove the export feature again, so as not to cause any more confusion as long as this patch is not complete. I'll open an issue for that to discuss further. Edit: Opened #2401787: Remove Features export while Features import is not available.

I've had a go at implementing this for features as we needed it for a client.

It only exports the policies themselves as we are already using strongarm to export the system variables.

The patch uses the policy name as the key as this appears to be unique and transferable to different environments.

I've put some helper functions, such as load_by_name() and policy_save(), into password_policy.features.inc which could probably be better in different files.

I forgot the file.

I have tried patch #50 from John Cook, but my policy was not exported with features. I could not find it anywhere.

I did use patch #50 ONLY. Do I need this in conjunction with another patch to work ?

#50 worked for me. it required a cache clear to have the password policy show up in features, and it exported into the feature.features.inc file.

Thanks for patch in #50, I plan to try this out soon.

I made two minor changes in this patch.

Typo:

$export['dependencied']['features'] = 'features';

Should be "Saves". (Verb in function short descriptions should be third person singular per coding standards.)

* Save a policy to the database.

password_policy_policy_save() uses $form_state, but $form_state is undefined.

Ideally this would also import/export not just the policies, but also the Password Policy configuration that is stored in database variables? For example, the "Admin (UID=1) password expires." setting?

It is definitely useful for the policies alone--just trying to understand what the end goal is.

@AohRveTPV: there is no need to export variables, since this task is done by strongarm

Thanks, valthebald. Between Strongarm and this patch, maybe all the Password Policy configuration is covered.

Looking at #53 further, code deduplication is needed between it and the rest of the module. I'm working on it now.

Want to complete this task first: #2647176: Rename 'policy' field to 'constraints'

The code in the patch is confusing because the constraints of a policy are stored in a field named "policy", so you get confusing naming like $policy['policy']. $policy['constraints'] would be much clearer. (This is not the patch's fault; the existing code is this way.)

New patch to work with changes committed due to #2647176: Rename 'policy' field to 'constraints'.

Still needs work per #57.

.

Currently requires first applying patch in #2647176: Rename 'policy' field to 'constraints'.

Changes versus #59:
1. Remove duplication between password_policy.features.inc:password_policy_save_policy() and password_policy.admin.inc:password_policy_admin_form_submit(). There is now a password_policy_save_policy() function in password_policy.module used by both the Features integration and the "admin form".

2. Change return array('password_policy_features_default_policy' => $code); to return array('password_policy_features_default_policies' => $code);, because $code can contain multiple policies.

3. Rename password_policy_policy_load_by_name() and _password_policy_load_policy_by_name() functions, which have confusingly similar names. Move them to password_policy.module.

4. Correct a variable name. (Was $pid when it actually contains a policy name.)

To-do:

1. Simplify this code:

/**
 * Implements hook_features_export_options().
 */
function password_policy_features_export_options() {
  $policies = array();

  $result = db_select('password_policy', 'p', array(
    'fetch' => PDO::FETCH_ASSOC,
    'target' => 'slave',
  ))
    ->fields('p', array(
      'pid',
      'name',
      'enabled',
      'description',
      'created',
      'weight',
    ))
    ->orderBy('weight')
    ->execute();

  foreach ($result as $row) {
    $policies[$row['name']] = $row['name'];
  }

  return $policies;
}

It is only retrieving policy names, so we can probably use a simpler query.

2. Remove duplication between _password_policy_load_policy_from_db_by_pid() and _password_policy_load_policy_from_db_by_name().

Please review/test.

Completed to-dos in #61. There is still some code duplication that I couldn't figure out how to remove simply:

**
 * Loads the policy with the specified id.
 *
 * Attempts to load the policy from a static cache variable. If not found,
 * loads the policy from the database.
 *
 * @param int $pid
 *   The policy id.
 *
 * @return array|false
 *   A populated policy array or FALSE if not found.
 */
function _password_policy_load_policy_by_pid($pid) {
  static $policies = array();

  if (is_numeric($pid)) {
    if (isset($policies[$pid])) {
      return $policies[$pid];
    }
    else {
      $policy = _password_policy_load_policy_from_db(array('pid' => $pid));
      if ($policy) {
        $policies[$pid] = $policy;
        return $policy;
      }
    }
  }
  return FALSE;
}

/**
 * Loads the policy with the specified name.
 *
 * Attempts to load the policy from a static cache variable. If not found,
 * loads the policy from the database.
 *
 * @param string $name
 *   The name of the policy.
 *
 * @return array|false
 *   A populated policy array or FALSE if not found.
 */
function password_policy_load_policy_by_name($name) {
  static $policies = array();

  if (isset($policies[$name])) {
    return $policies[$name];
  }
  else {
    $policy = _password_policy_load_policy_from_db(array('name' => $name));
    if ($policy) {
      $policies[$name] = $policy;
      return $policy;
    }
  }
  return FALSE;
}

In the exported code, the roles for the policy appear like this:

      'roles' => array(
        'authenticated user' => 'authenticated user',
      ),

But it should probably just be an indexed array like this:

      'roles' => array(
        'authenticated user',
      ),

Changes:
- Made improvement mentioned in #64.
- Fixed serialization bug.
- Added type hinting for a function.
- Made a function short description consistent with others.

I think there is a problem with exporting policies weights.

tunprog, thanks for reporting the problem. I think this patch should fix it, but I have not yet had a chance to test. password_policy_save_policy() was simply not saving the weights, so I added to its db_update() and db_insert() calls:

'weight' => !empty($policy['weight']) ? $policy['weight'] : 0,

I've done a bit of testing and it seems to be working real well, but I'm really curious when these changes are planned to be released!

Thanks for testing, fubarhouse, and glad it is working.

Will probably commit once there is a little more review/testing. Especially would like to know if the weights are working properly. Then there will be a development release with the feature.

There are no firm plans for when the next stable release will be made. I think it might be good to wait a month after this feature is added at least, so there is more chance for testing. If the Features integration does not work properly, password policies could be altered after export/import in a way that affects security. For instance, if a constraint value were lost, that could be bad.

I tested patch #67 and it fixes the policies weights export issue.
thank you.

About the commit below:

January 10, 2016 3:24

Commit a3884b8 on 7.x-1.x
by AohRveTPV
Issue #2647176 by AohRveTPV, eelkeblok: Rename'policy' field to 'constraints'

Now the password_policy-7.x-1.x-features_integration-985758-67.patch (#67) is broken when I try to enable my custom feature.

The root cause is rename the field 'constraints' to 'policy', because in the insert is not changed.

Fix:
- Rename 'constraints' to 'policy' in password_policy_save_policy function (insert and update);
- Remove unnecessary serialize in password_policy_save_policy function (insert and update);

Sorry about my last patch. I was checking with the stable version.

* The patch #72 is merged with stable version.
* I tested the patch #67 and is working normally.

Setting back to needs review. To be clear, #67 is the patch to test against.

Issues:
- When the user try update or insert some password_policy with same name, the page broken.
- When is inserted new password_policy, not exist "enable" variable in array, only when imported by features.

Fixes:
- When the user try update or insert some password_policy with same name, the page broken.
- When is inserted new password_policy, not exist "enable" variable in array, only when imported by features.

- When the user try update or insert some password_policy with same name, the page broken.

Thanks, marlonleandropereira, for reporting this problem. It seems to be unrelated to this feature request, because it occurs without any patch applied. So I opened a separate issue for the bug:
#2677500: Adding policy with same name as existing policy causes error

Strangely, comments #77 and #79 disappeared. Perhaps drupal.org thought it was spam since you are an unconfirmed user. So, please repost your patch.

- When is inserted new password_policy, not exist "enable" variable in array, only when imported by features.

I can't reproduce this. With the patch in #67, when a password policy is added, enabled is set to 0 by _password_policy_admin_form_save_policy().

Are you sure this is a problem? Could you give steps for reproducing it?

Setting back to "Needs review" as I do not see any problems with #67.

There is the potential problem discussed in #81, but I cannot reproduce it.

Myself and my team are rather eager to see this functionality come to light - what can I do to help push this along?

The status of the issue is "needs review", so if you can review the patch (most recent one is #67, make sure the fix for #2647176: Rename 'policy' field to 'constraints' is applied), that would be great. If you have and don't find any problems, you can set it to Reviewed and tested by the community (I think that should be OK given that the patch already went through various hands and has been reviewed by others too). Another option is to update the issue summary, outlining this (use the issue summary template as a starting point). This patch depending #2647176: Rename 'policy' field to 'constraints' is important.

**Edit:** Actually, #2647176: Rename 'policy' field to 'constraints' has already been committed, so you can forget most of what I said about it.

Worked for me against the latest dev version.

Just gotten around to testing it, and all good.
Cannot replicate #67.

I see Liam has already provided feedback, so thanks mate!
Now we wait for a release :)

Will commit soon, thanks.

Hi. There is reworked version from #67. Please can you review it?
It fixes broken resaving of existing policies. And also there are some changes in features_export hook.

Updated previous version of patch: now it add to dependencies all modules that contains according to policies roles.

Thank you for the catch, @vilepickle.
Looks like something gonna wrong when patch was created.
There is reworked patch from #92 with correct password_policy_features_rebuild() function.
Also interdiff attached.

Sorry, upload interdiff in wrong format.
Updated.

Patch from #98 throws notices:

Notice: Undefined index: constraints in _password_policy_load_policy_from_db()

Hello @kristiaanvandeneynde, I'll check it and try to fix failing tests.

For me, last patch is not applicable to last public release of the module.

+1

Is there anything I could help speeding up the process?

@koosvdkolk,

Based on the recent comments, one way to speed up the process would be to update the patch so that it applies to the latest dev release and so that the tests pass.

The tests could have been failing due to an unrelated issue that is now fixed: #2857748: Password expiration tests do not reliably pass.

Thanks for the continued work on the patch. It sounds like the notices reported in #105 need investigation.

#98 has 4 coding standards errors and 3 Drupal practices warnings per Coder. Needs a little work.

@AohRveTPV thank you for your response

Fixed code standard errors.

About strange notices reported in #105. I've checked code and looks like that the only situation when these notices can be generated is when user have no 'constraints' column in {password_policy} table. (This true only for stable version of module or if updates from dev version weren't executed).

Coder 8.x-2.x still found some issues. Corrected in this patch.

sumthief, could you explain what the following code is doing, particularly the else block:

    $map = features_get_default_map('user_role', 'name');
    foreach ($policy['roles'] as $rid) {
      $role = user_role_load($rid);
      if (!in_array($role->name, array_keys($map))) {
        $export['features']['user_role'][$role->name] = $role->name;
      }
      else {
        $dependency = $map[$role->name];
        if (!isset($export['dependencies'][$dependency])) {
          $export['dependencies'][$dependency] = $dependency;
        }
      }
    }

I have an idea what the else block is doing but am not sure. Thank you.

I factored some code into separate functions, added some inline comments, and clarified some variable names. No functional changes, but I think this makes the code a bit easier to read/understand.

Re the variable name changes: "save" was being used in two senses in the same function. In one sense, referring to exported policies as "saved policies". In the other, password_policy_save_policy(), which stores a policy to the database.

@AohRveTPV,

As I remember I've faced with problem that generated feature didn't include other features (exporting roles associated with selected policies) as dependencies.

Thanks, but I have to admit I am still hazy on what that else block does.

For the sake of testing, what can I do in Drupal to cause the else block to be executed?

Maybe I understand now. The roles themselves may have dependencies, so the else block in #114 exports the dependencies of the roles.

This seems like an infinite problem: What if the dependencies of the roles themselves have dependencies? I'm not sure why Password Policy should concern itself with dependencies beyond its immediate dependencies.

Please correct me if I'm misunderstanding. I have limited experience with Features.

Please review and test. It is important that this patch be tested, because export/import of password policies not working correctly has security implications.

Patch removes the aforementioned else block and adds an inline comment.

If the else block is needed, someone will have a problem with this patch and can give clarity to why it is needed.

I'll try to get some testing in tonight, I would've assumed this would have progressed by now.

Is there anything blocking this from release other than additional testing?

I'll try to get some testing in tonight, I would've assumed this would have progressed by now.

Thanks.

Is there anything blocking this from release other than additional testing?

It just needs testing. #119 hasn't gotten any feedback. It seems important that this be tested before release because if it doesn't work properly, sites could be made less secure.

Also unsure about the else block mentioned in #114, which I removed in #119.

Patch in #119 works as expected: import/export tested.
Tested with the latest version of Features and Password policy itself.

Please review/test the Features functionality now in 7.x-1.x-dev if you haven't already, and report back whether it works. I will release only once it is tested more and some time has passed.

Thanks, RoSk0, for testing #119.

I credited those who provided patches building on John Cook's new patch in #50. Please let me know if I missed crediting anyone. Thanks to everyone who has reviewed/tested iterations of that patch.

Out of the tests I've performed on #119, it works flawlessly.

Thanks, will plan to release in a week or two:
#2869115: Release 7.x-1.13