Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff_18-22.txt | 673 bytes | alison |
#22 | nodeaccess-fix-userreference-2099871-22.patch | 7.31 KB | LeDucDuBleuet |
|
Comments
Comment #1
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedPlease 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.
Comment #2
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedNo 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.
Comment #3
yosia_ken CreditAttribution: yosia_ken commentedHi, 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.
Please help!
Comment #4
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedI 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).
Comment #5
Phirames CreditAttribution: Phirames commentedThis can help with user reference
Comment #6
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedCan you please re-roll that patch against the dev version?
Comment #7
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedComment #8
ChaseOnTheWebThe 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:
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.
Comment #9
ChaseOnTheWebPatch in #8 was against 7.x-1.4, rerolled against -dev
Comment #10
kenorb CreditAttribution: kenorb commentedComment #13
Ludo.RI confirm patch #8 works against 1.4 version
Comment #14
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedThis 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.
Comment #15
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedComment #16
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedSince 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.
Comment #17
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedAs 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.
Comment #18
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedHere 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.
Comment #19
alisonCircling 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?
Comment #20
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedWelcome 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.
Comment #21
alisonHi 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:
(@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?)
Comment #22
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedHere are the steps to reproduce/test :
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!
Comment #23
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedComment #24
markie CreditAttribution: markie at Mediacurrent commentedReviewed and tested on simplytest.me. Was not able to replicate the problem.
Comment #25
alisonThank 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:
More info about the need to uninstall -- potentially a starting point for tweaking the implementation? -- from @LeDucDuBleuet (comment #18):
..........
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)
Comment #26
alisonPSYCH on the interdiff -- here it is.
Comment #27
alisonRelease blocker for 7.x-1.8.
Comment #28
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedAbout 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.
Comment #29
alison@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!
Comment #30
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedOk 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!
Comment #31
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedOups... :-)
Comment #32
alisonPinging myself to come back to / review this patch.