This seems like a pointless optimization to me:

function view_unpublished_get_configured_types($reset = FALSE) {
  static $types = array();
  $types = (empty($types) || $reset == TRUE) ? node_permissions_get_configured_types() : $types;
  return $types;
}

This is just a wrapper around node_permissions_get_configured_types() which is a very light weight function that does not get called often. I suppose you are caching this because you are calling it twice. I actually think this does not warrant the additional complexity.

But perhaps I am missing something?

CommentFileSizeAuthor
#1 1494652-1-view_unpublished-premature_optimization.patch1.66 KBpfrenssen

Comments

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Active » Needs review
StatusFileSize
new 1494652-1-view_unpublished-premature_optimization.patch1.66 KB

In case you agree, here is a patch that removes the optimization. If you do not agree, then just mark this with "won't fix" ;)

Chaulky’s picture

Chaulky commented

Not only is it a premature optimization, but it also interferes with the flow of node types/permissions. For example, I just ran into an issue using View Unpublished in multiple Features modules that are enabled during an automated site install. Because of the static variable here, the list of permissions and the modules that provide them was incomplete when my Features module attempted to rebuild the related permissions. Removing the optimization in favor of the call to node_permissions_get_configured_types() as in this patch resolved the issue.

The details of exactly how this simple change affected the rest of the Drupal/Features system would take too long to write here, but the upshot is that this patch fixes the issue during automated installation of multiple features that use View Unpublished permissions.

Chaulky’s picture

Chaulky commented
Status: Needs review » Reviewed & tested by the community

I used this patch for reasons noted above. Everything continues to work correctly. RTBC

entendu’s picture

entendu commented

Agree on the patch. Was going to commit tonight but my local environment's acting up. Bear with me...

entendu’s picture

entendu commented
Status: Reviewed & tested by the community » Closed (fixed)

Committed to dev.

magicmyth’s picture

magicmyth commented

I know this bug is resolved but I thought I should point out that IMHO this function should have been marked "Deprecated" instead of being removed. It was a publicly accessible function by convention (I know Drupal 7 does not have private functions for modules) as it was not prefixed with an underscore. I actually made a call to this function in one of my modules on a client site and after upgrading View Unpublished the page naturally broke because the function no longer existed. Such changes should only occur in major version updates (where breaking API changes can occur). Not point releases.

It might be worth considering adding it back for the 1.x series but its not a major issue for me. Just being pedantic ;)

pfrenssen’s picture

pfrenssen
Location Sofia
commented

You're absolutely right, this was a public function and should have remained in the code, with a deprecation warning output to watchdog whenever it was used. I'm sorry for the trouble this has caused you.