Problem/Motivation

@todo: Add a summary and steps to reproduce!
For now, please see comment #22 for instructions for testing the patch (and as such, an implied explanation of how to encounter the problem).

Remaining tasks

Review the patch on a site without user reference, to verify there aren't any side effects.

Original report by [username]

    Notice: Undefined index: uid in nodeaccess_delete_user_reference() (line 1234 of /var/www/drupal/sites/all/modules/nodeaccess/nodeaccess.module).
    Warning: array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load() (line 173 of /var/www/drupal/includes/entity.inc).
    Warning: array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->cacheGet() (line 350 of /var/www/drupal/includes/entity.inc).
    Notice: Trying to get property of non-object in nodeaccess_delete_user_reference() (line 1235 of /var/www/drupal/sites/all/modules/nodeaccess/nodeaccess.module).
    Notice: Undefined index: uid in nodeaccess_insert_user_reference() (line 1193 of /var/www/drupal/sites/all/modules/nodeaccess/nodeaccess.module).
    Notice: Undefined index: uid in nodeaccess_insert_user_reference() (line 1211 of /var/www/drupal/sites/all/modules/nodeaccess/nodeaccess.module).

when i try to save a content type with a user reference field this are the error displayed. need help.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vlad.pavlovic’s picture

Please try the dev version and let me know if it works. I believe that this was repaired as part of #2037225: User referenced by node cannot have access to node.

Let me know if 7.x-1.x has this issue as well. If not, I will tag it as 7.x-1.1 shortly. Once I close a few more issues, I feel that there have been enough improvements to warrant the change.

vlad.pavlovic’s picture

Issue summary: View changes
Status: Active » Closed (cannot reproduce)

No response for over 2 months, if this is still an issue, please open a new issue. I am just trying to clean up the Issue Queue.

yosia_ken’s picture

Status: Closed (cannot reproduce) » Active

Hi, I found this issue on 7.x-1.4 as well. The error below displayed after saving or creating a node with the node_reference field.

Warning: array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load() (line 173 of /var/www/drupal/includes/entity.inc).
    Warning: array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->cacheGet() (line 350 of /var/www/drupal/includes/entity.inc).

Please help!

vlad.pavlovic’s picture

Status: Active » Closed (cannot reproduce)

I don't think the errors that you are seeing are connected to this module any longer. Note that there are no nodeaccess errors (as there were above).

Phirames’s picture

Status: Closed (cannot reproduce) » Active
FileSize
2.24 KB

This can help with user reference

vlad.pavlovic’s picture

Can you please re-roll that patch against the dev version?

vlad.pavlovic’s picture

Version: 7.x-1.0 » 7.x-1.x-dev
ChaseOnTheWeb’s picture

The problem is still in this module. nodeaccess_[insert|update]_user_reference() assumes the output of field_get_items is just an array of user IDs, when in fact it is an array of field items. Thus, user_load_multiple is getting called with an array like this:

array(
  0 => array(
    'uid' => 101,
  ),
  1 => array(
    'uid' => 252,
  ),
);

Which apparently isn't validated by Drupal and just chokes further down in the entity system. We don't need the array walking logic of #5, because the user_reference's field structure is always the same. Just use array_map().

Patch attached. The patch also addresses a related issue, that nodeaccess_[insert|update]_user_reference() is still looping over fields that aren't enabled in the nodeaccess settings.

ChaseOnTheWeb’s picture

Patch in #8 was against 7.x-1.4, rerolled against -dev

kenorb’s picture

Status: Active » Needs review

The last submitted patch, 8: nodeaccess-fix-userreference-2099871-8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: nodeaccess-fix-userreference-2099871-9-dev.patch, failed testing.

Ludo.R’s picture

I confirm patch #8 works against 1.4 version

LeDucDuBleuet’s picture

Status: Needs work » Needs review
FileSize
2.7 KB

This patch is still needed for userreference access to work.

I simply re-rolled the same patch as in #8 & #9 against current dev.

It would be great if someone could review it so it can be committed in the next release.

Thank you.

LeDucDuBleuet’s picture

LeDucDuBleuet’s picture

Version: 7.x-1.6 » 7.x-1.x-dev
Status: Needs review » Reviewed & tested by the community

Since I am not the original author of this patch, I am taking the liberty to mark it as RTBC myself. If this is not appropriate, please feel free to put it back to "Needs review".

I tested it extensively for a project using nodeaccess + user_reference for content access control. It is working properly with this patch and I did not notice any side effect.

Thank you.

LeDucDuBleuet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.04 KB

As it turns out with all the other needed patches on 1.6 and using PHP7, the patch in #14 was not working at all anymore.

Here is a new patch to review, my tests were all successful but bear in mind that I had also applied all the other patches linked in #3033025: [META] Nodeaccess: Multiple issues with version 1.6.

The grants for user references are now much simpler and cleaner with this new approach. Be careful if you have custom grants for users already in the nodeaccess table. If possible, I strongly suggest you empty this table before rebuilding the permissions to avoid duplicates. If you don't know how to do it, I suggest you completely uninstall this module before installing it again with this patch included. Of course, your mileage may vary...

Thanks.

LeDucDuBleuet’s picture

Here is a new version of my patch against current dev which shows the user reference grants on the form for users referenced in the node. This is to explicitly show the grants for these referenced users on the form when enabled in the content type. These grants will always pop back even if the keep box is unchecked, of course. This prevents adding duplicates when referenced users are hidden.

Same warning applies. The grants for user references are now much simpler and cleaner with this new approach. Be careful if you have custom grants for users already in the nodeaccess table. If possible, I strongly suggest you empty this table before rebuilding the permissions to avoid duplicates. If you don't know how to do it, I suggest you completely uninstall this module before installing it again and start over with this patch included. Of course, your mileage may vary depending on your specific use case.

Thanks.

alison’s picture

Circling back -- thanks for the patch and updates, @LeDucDuBleuet! Did you happen to check whether the patch is also ok on PHP 5.x?

Would you like to plan to include this fix with 1.7, or hold off 'til after 1.7?

LeDucDuBleuet’s picture

Welcome to @alisonjo2786, I'll do my best to help! :-)

The patch passes the PHP 5.3 tests and looking at it, I don't see any potential problem but I did not have time to test it on a PHP5 setup.

This patch makes the module a little cleaner even for sites not using user_reference module. We do not use hook_node_update() anymore and the whole approach is in line with the rest of the nodeaccess module. But there are not a lot of sites using nodeaccess with user_reference module and the patch was not reviewed by another than myself.

Also, for people using user_reference with nodeaccess 1.6, the integration is not working at all so this patch is absolutely needed. In that case, my warning applies to empty the nodeaccess table beforehand or completely uninstall the module first and start over with the patch.

So if someone could review this patch for us, I think we should try to include it as soon as we can. If possible, even in 1.7.

Do we need a new patch against current dev?

Thank you.

alison’s picture

Issue summary: View changes
FileSize
5.44 KB

Hi folks! I verified that the patch still applies against the latest code (Rerolling patches) -- yay!

Also: attached is an interdiff for #14 (a straight reroll of #9) to #18.

Next steps:

  • This patch needs review. If you have specific "steps to reproduce" the problem being addressed by the patch, please add them to the issue summary to help with testing.
  • We also need testing steps for folks (like me) who aren't using user_reference with nodeaccess, to check for possible side effects. Chime in with such steps if you've got 'em!

(@LeDucDuBleuet I know you can't be the reviewer, but could you maybe add "steps to reproduce" to the issue summary -- aaaand maybe some best-guess testing steps for folks who aren't using user_reference, based on your understanding of the code being changed?)

LeDucDuBleuet’s picture

Here are the steps to reproduce/test :

  1. Install the References module and its User Reference sub-module : https://www.drupal.org/project/references
  2. Add a new User Reference field to a content type :
    User reference field
  3. Then you will get a new section for this field in the content type at admin/config/people/nodeaccess :
    User ref section
  4. You have to enable the user reference field and grant some permissions;
  5. Rebuild the permissions.

Then, when you publish a content of this type, the user referenced in the field will get the permissions set in admin/config/people/nodeaccess.

Without the patch, it will not work and it is hard to say exactly what will happen. And with the patch, well, it works as intended.

Be sure to uninstall the module completely before trying the patch if you did try before without the patch or simply a previous patch. You can also empty the nodeaccess(not the node_access) table in order to flush the old nodeaccess grants before trying again from the start.

As for side-effects, I don't see them possible if you are not using the User Reference module since there are checks for module_exists('user_reference') everywhere in the source.

Also, the grant_delete permission checkbox was not showing in the form since it was using drupal_render_children() instead of drupal_render() for this option. Here is a new patch including this little extra fix. I tested it to be working well.

Let me know if you need anything else to help the testing!

LeDucDuBleuet’s picture

markie’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested on simplytest.me. Was not able to replicate the problem.

alison’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Thank you for all the work and context/instructions, @LeDucDuBleuet, and thank you for the review help, @markie!

..........
And yet, I need to change the status back to "needs work" -- not that anything was done wrong, we just aren't in a place to put the changes into the module, YET:

  • We seem to have a solid idea of what's wrong -- we have code that gets rid of the problem, and that's always a good indicator of knowing what's wrong :)
  • Now we need to rework the implementation so that the fix can be applied without needing to uninstall nodeaccess (per instructions in #22 and #14) -- i.e. to make the fix work for existing users, as well as new users.
  • (Also, we need to check for side effects on non-user-reference usage -- I still haven't gotten around to that.)

More info about the need to uninstall -- potentially a starting point for tweaking the implementation? -- from @LeDucDuBleuet (comment #18):

Be careful if you have custom grants for users already in the nodeaccess table. If possible, I strongly suggest you empty this table before rebuilding the permissions to avoid duplicates. If you don't know how to do it, I suggest you completely uninstall this module before installing it again and start over with this patch included. Of course, your mileage may vary depending on your specific use case.

..........
I will, however, add a note to the project page (and the 7.x-1.7 release notes) to let users know that if uninstalling nodeaccess is an option for them, there's an in-progress fix to the user reference bug.

..........
@LeDucDuBleuet if I misunderstood your instructions, my apologies -- just let me know! Thank you again for your work on this issue!

..........
(less importantly: attached is an interdiff of #18 to #22)

alison’s picture

FileSize
673 bytes

PSYCH on the interdiff -- here it is.

alison’s picture

Issue tags: +Release blocker

Release blocker for 7.x-1.8.

LeDucDuBleuet’s picture

About the needed uninstallation of the module, this is the case only if you tried previous patches with the User References module used to grant permissions. Sorry if that was unclear. If you did not try previous patches, I believe the uninstall is not needed but more tests are needed to confirm that.

Also, I don't see how we could rework the implementation since it is not possible to know where the grants are coming from in the old nodeaccess table... I guess we could write an update function that looks for nodes with user references field activated for granting access in order to clean up the nodeaccess table for these specific nodes? But since there are not a lot of sites using this feature and the D7 EOL approaching fast, I wonder if it is worth the effort?

As for side effects, as I said, I don't see any possible since the code is isolated and unused if the user reference sub-module is not installed.

Since this patch simplifies the module and makes it cleaner for everybody, I do think that we should include it asap with a warning for people using the user references sub-module to grant access. In fact, this feature has not been working for a long time anyway so it makes sense to me to deliver a working module even though this patch might require some work for these sites...

Thank you.

alison’s picture

Issue summary: View changes
Status: Needs work » Needs review

@LeDucDuBleuet Ahhhhh ok, I definitely misunderstood, I thought the uninstall was necessary for using this patch -- I gotcha now, thank you for clarifying for me!

As for re-implementing, no need, the only reason I said that was b/c I thought the uninstall was necessary, so I was thinking we should re-implement so that module users don't have to uninstall first :)

Side effects -- makes total sense, I'll still put "review patch without user reference to verify no side effects" as a "remaining task," just to be extra sure, but what you're saying makes sense to me.

Thanks so much!

LeDucDuBleuet’s picture

Status: Needs review » Needs work

Ok I've tested this feature again and I can confirm that if you are coming from the stable versions path, there is no need to uninstall the module. The uninstallation is needed only if you tried previous patches in this issue and that is not a problem since you are not supposed to do that on a production site anyway. ;-)

So I've tested from 1.6 to 1.7 and then applying the patch and nothing was messed up in the upgrade process. This patch still applies cleanly to 1.7 and makes the user references feature working fully and completely! Yeah! :-)

Of course, someone else than me have to review this as well but my tests are conclusive!

Thank you!

LeDucDuBleuet’s picture

Status: Needs work » Needs review

Oups... :-)

alison’s picture

Pinging myself to come back to / review this patch.