Compatibility with Workflow 2.5 API changes

Had some trouble around like 318: the check isn't necessary, since we're only in that alter function if we're on a node form anyway. We just need to check for the workflow field.

  // if this isn't a workflow field, get out.
  if (null == field_get_items('node', $node, 'field_workflow')) {
    return;
  }
  $sid = workflow_node_current_state($node);

It looks like this has includes another patch related to field visibility and view modes.

Someone needs to re-roll this with only the changes for the current issue. It's not acceptable to include unrelated changes in your patch.

@Renee: That only works if the site builder named the field "field_workflow".

Well, ok, then use "if (isset($node->workflow))" instead as the test... but the rest of the if statement at line 316 (checking entity type) isn't necessary, and the additional arguments to the function just plain don't work.

Ok, yeah, what's needed in both functions, the node_alter AND fields_access, should be safely covered by the following (lines 318 and 597ish)

<?php
    $sid = NULL;
    // Only relevant for content with Workflow.
    if (isset($node->workflow)) {
        $entity_type = 'node'; // This hook is only for nodes.
        $field_name = ''; // This hook is only for workflow_node.
        $sid = workflow_node_current_state($node, $entity_type, $field_name);
    } else if (isset($node)) {
      list($entity_id, , $entity_bundle) = entity_extract_ids($entity_type, $node);
      $fields = _workflow_info_fields($node, $entity_type, $entity_bundle);
      if(isset($fields)) {
        $sid = workflow_node_current_state($node);
      }
    }
?>

This covers the edit case on the fields_access function, because that is delivered via the $node->workflow method, and it will cover folks using workflownode also.

For the node_alter version, replace $entity_type with 'node'.

ETA: now addresses the "what if the field isn't named field_workflow" issue.

That sounds very nice. Patch rolled and commited to dev. Stable release created. Thank you for your great work ! :)

Question: Why do you need the else if (isset($node)) { line?

If $node is unset, the if (isset($node->workflow)) { will blow-up first.

@hbemtec: I would have let this issue go to RTBC before jumping to commit... this code still needs work.

Since _workflow_info_fields() always returns an array (even an empty one), the line if(isset($fields)) { makes no sense. It should be if(empty($fields)) {, should it not?

You're right, Renee's code can be simplified into this :

  $sid = workflow_node_current_state($node);

as workflow_node_current_state() tries to find the field the same way Renee does.

The $field_name parameter was introduced into workflow_node_current_state() in case a content type has multiple workflows on it (as 2.x releases of Workflow made workflows supported by fields).

I think that, in order to cover every single workflow field (in case there are more than one), we should do something like that :

/**
 * Implements hook_field_access().
 */
function workflow_fields_field_access($op, $field, $entity_type, $node, $account) {
  if ($entity_type == 'node' || !$node) {
    $sid = NULL;
    $fields = _workflow_info_fields($node, $entity_type);
    
    // If there are no fields
    if(0 === count($workflowFields)){
      return TRUE;
    }
    
    // If there is one workflow that gives access to the field, access is granted
    foreach($workflowFields as $workflow_field_name => $workflow_field_info){
      $sid = workflow_node_current_state($node, 'node', $workflow_field_name);
      if (!is_numeric($sid)) {
       $type_map = workflow_get_workflow_type_map_by_type($node->type);
       $sid = workflow_get_creation_state_by_wid($type_map->wid);
      }

      // Check for visible/editable flags.
      list($visibles, $editables) = _workflow_fields_compute_permissions($sid, $node->type, $node, $op);
      if (isset($visibles[$field['field_name']])) {
        $isAccessible = ($op == 'view') ? $visibles[$field['field_name']] : ($visibles[$field['field_name']] && $editables[$field['field_name']]);
        if($isAccessible){
          return TRUE;
        }
      }
    }
    
    return FALSE;
  }
  else {
    return TRUE;
  }
}

The same method can be used for workflow_fields_form_node_form_alter().
What do you think about it ?

Most of this looks reasonable. Out of curiosity, why the check for a numeric return from workflow_node_current_state()? It is supposed to always return a numeric ID. It would be odd for it to return a string.

There's also some minor code clean-up / formatting to attend to (spacing after conditionals -- after if and foreach, etc -- and the naming of isAccessible).

Well, okay, workflow_node_current_state() can technically also return FALSE true, apparently. Still, not sure what the next few lines are trying to do.

The next lines are directly copied from the function as it has always been implemented.
All I changed is the "whitelist" approach : I consider that, when one or multiple workflows exist, access to a field is denied until at least one workflow specifically grants it.

You're right for the formatting, I did it quickly just to give an idea of my approach and have some feedback. :)

GuyPaddock: isset() is very specific. if (isset($node->workflow)) might return false, but that doesn't mean there's a $node either; or, it might return false because there's a $node, too. If there's no $node, things don't end well when you try to do entity things on $node...

(This is nerdfun: http://kunststube.net/isset/)

But, simplified is even better!!

Here is a patch that does the following :
- implement the concept of whitelist I previously exposed for workflow_fields_field_access() in a way that follows the coding standards
- simplify workflow_fields_form_node_form_alter()'s initial sid find
- replaced all calls of workflow_get_creation_state_by_wid() that has been deprecated

To my mind, this patch is a good fix but clearly not a long-term solution, as Workflow Fields does not currently handle multiple workflows (except for workflow_fields_field_access()). That's why I think :
- we should go over this patch to have a stable fix (so your reviews are welcome)
- we should open another issue to make the module evolve to handle multiple workflows

On this project home page it states "The current stable release is NOT compatible with Workflow 2.0. Instead, use the development release.."

However, 7.x-1.0-beta2 contains reference to function _workflow_info_fields() which is defined in Workflow 2.0 but not Workflow 1.0.

Code change was implemented due to this issue.

This causes
Fatal error: Call to undefined function _workflow_info_fields() in [..]workflow_fields/workflow_fields.module on line 618

So, the stable release is no longer compatible with Workflow 1.0.

Solution: Either the home page text needs to change or the 7.x-1.0-beta1 should have remained the stable release?

@lagatixa : you're right, so here is an improved patch that does the previous AND remains compatible with Workflow 1.

I'm not sure it's the proper place to write this, but currently dev version is broken (with with workflow 2+) from the time this was committed.

It still requires the patch from #2148935: Make module compatible with workflows defined through "Workflow field" module

@Zekvyrin : Yep, the patch from #2148935: Make module compatible with workflows defined through "Workflow field" module is a good complement. I'll commit it when this issue will be solved.
Apply the patch from #18 and tell me if it is still broken.

Notice: Trying to get property of non-object in workflow_fields_field_access() (line 604 of [..]/sites/all/modules/workflow_fields/workflow_fields.module).

Line 604: $type_map = workflow_get_workflow_type_map_by_type($node->type);

Issue is when entity_type is "profile2" or using node/entity reference fields to node/entity that don't have workflows.

NB: $entity (a.k.a. $node in the module code) is actually optional in hook_field_access($op, $field, $entity_type, $entity, $account)
REF: https://api.drupal.org/api/drupal/modules!field!field.api.php/function/h...

In 7.x-1.0-beta1 lines 594-598 had the following

if ($entity_type == 'node' || !$node) {
    $sid = workflow_node_current_state($node);
    if (!$sid) {
      return TRUE;
    }

In 7.x-1.0-beta3 lines 588-590 this has been changed to:

if ($entity_type == 'node' || !$node) {
    $sid = NULL;
    $workflow_fields = array('');

Since line 604 in beta3 resets $sid to FALSE for fields without workflow anyway
$sid = workflow_node_current_state($node, 'node', $workflow_field_name);
Changing beta3 to:

if ($entity_type == 'node' || !$node) {
    $sid = workflow_node_current_state($node);
    if(!$sid){return;}
    $workflow_fields = array('');

stops the issue but does it impact adversely on the rest of the code?

As mentioned previously, I'm using Workflow 1.0 and the changes to the beta are for Workflow 2.0.

+  $sid = workflow_node_current_state($node);

it should be change to:

  $sid = NULL;
    // Only relevant for content with Workflow.
    if (isset($node->workflow)) {
        $sid = workflow_node_current_state($node);
    }

it not error "Workflow 0 cannot be loaded. Contact your system administrator." would produce.

I am using workflow 2.8 + workflow fields latest dev and get the following error and notice

When clicking on workflow fields at admin/config/workflow/workflow/manage//states

PHP Fatal error: Call to undefined function workflow_get_workflow_states_by_sid() in /var/www/vhosts/energiemarkt.discount/httpdocs/sites/all/modules/workflow_fields/workflow_fields.module on line 40, referer: http://domain.com/admin/config/workflow/workflow/manage/lead_status/states

Visit normal config page of a entity field

Notice: Trying to get property of non-object in workflow_fields_field_access() (line 604 of /httpdocs/sites/all/modules/workflow_fields/workflow_fields.module)

@fox-01, It seems you are using workflow_field (the workflow submodule), not workflow_node. I don't think workflow_fields (this module) supports that at all.

Yes i am using the submodule workflowfield. Should i create a new feature request in the issue queue? I think this should be covered by this module / patch here too because its the new way how workflow works in the branch 2.x.