Problem/Motivation

Around line 226 in the node_access_example.module file, specifically in the node_access_example_node_access function we have the following line of code:

    drupal_set_message(t('Access to node @nid allowed because requester name (@name) is specifically allowed', array('@name' => $node->name, '@uid' => $account->uid)));

There are two replacement variables in the translation string, namely @nid and @name, but the variable array being passed have a different set of variables, namely @name and @uid.

Proposed resolution

The @uid variable '@uid' => $account->uid) should be replaced with '@nid' => $node->nid

Remaining tasks

  • Correct Code
  • Submit and test patch
CommentFileSizeAuthor
#15 2708763-15.patch1.91 KBsophiavs
#4 interdiff.txt970 bytesnavneet0693
#4 examples-fix_var_and_logic-2708763-4.patch1.2 KBnavneet0693
#2 examples-fix_var_and_logic-2708763-1.patch1.18 KBjpdippenaar

Issue fork examples-2708763

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jpdippenaar created an issue. See original summary.

jpdippenaar’s picture

jpdippenaar commented
StatusFileSize
new examples-fix_var_and_logic-2708763-1.patch1.18 KB

Replaced the variable '@uid' => $account->did with '@nid' => $node->did.

When I tested the code, I realised there may be a problem with the logic in the if statement as well. In specific !empty($node->private) did not make sense. If the variable was set to false, $node->private would evaluate to "0", which is not empty. Someone needs to verify the new logic.

jpdippenaar’s picture

jpdippenaar commented
Status: Active » Needs review
navneet0693’s picture

navneet0693 commented
StatusFileSize
new examples-fix_var_and_logic-2708763-4.patch1.2 KB
new interdiff.txt970 bytes

Hi @jpdippennar,

Added && $node->private to check the value of $node->private. isset($node->private) will only check whether $node->private is set or not regardless of it's value being TRUE or FALSE adding && $node->private will also check value of it and executes the if condition accordingly.

jpdippenaar’s picture

jpdippenaar commented

Thans @naveet,

Makes perfect sense, and happy that at least I was not completely wrong about the original logic not working.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented

The following things are considered to be empty:

"" (an empty string)
0 (0 as an integer)
0.0 (0 as a float)
"0" (0 as a string)
NULL
FALSE
array() (an empty array)
$var; (a variable declared, but without a value)

So, I think we don't need that isset clause and empty check is fine

manav’s picture

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

Test Results : Pass.
The given patch is fine working for me.

Drupal 7 Version : 7.43
MySQL : 5.5.42
PHP : 5.6.10

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests 
  1. +++ b/node_access_example/node_access_example.module
@@ -223,8 +223,10 @@ function node_access_example_node_access($node, $op, $account) {
+  if (($op == 'view' || $op == 'update') && (!empty($account->name) && isset($node->private) && $node->private && $account->name == 'foobar')) {

    $node should always have a private property. Something's very wrong somewhere else if it doesn't. So we can just say $node->private without the isset(). 

  2. +++ b/node_access_example/node_access_example.module
@@ -223,8 +223,10 @@ function node_access_example_node_access($node, $op, $account) {
+    drupal_set_message(t('Access to node @nid allowed because requester name (@name) is specifically allowed', array('@name' => $account->name, '@nid' => $node->nid)));

    Place the items in the array in the same order they are in the string. Also, we should use the node name instead of the nid.

    So: Use the name for both the account and the node.

  3. Also, there's a test for this access scheme in testNodeAccessBasic(), but it doesn't check the message. It should.
vicheldt’s picture

vicheldt commented
Assigned: Unassigned » vicheldt
Issue tags: -

vicheldt opened merge request !10

vicheldt’s picture

vicheldt commented
Assigned: vicheldt » Unassigned
Status: Needs work » Needs review

I created a MR because I wasn't able to apply the patch, and I also believe MR are easier to handle. So I added the changes that were in the last patch (comment #4) and the points that @Mile23 made, except the tests part, because I'm not sure how to proceed. If anything is incorrect here please let me know, I wanna learn more :)

vicheldt’s picture

vicheldt commented
Status: Needs review » Needs work
sophiavs’s picture

sophiavs commented
Assigned: Unassigned » sophiavs

Hi, i'll be doing the changes on tests :)

sophiavs’s picture

sophiavs commented

I submitted a new commit, but a new test fail in a file that i didn't change, i will still try to analyze the reason.

sophiavs’s picture

sophiavs commented
StatusFileSize
new 2708763-15.patch1.91 KB

I'm trying to upload a patch to confirm the test

sophiavs’s picture

sophiavs commented
Assigned: sophiavs » Unassigned
Status: Needs work » Needs review
lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented
Assigned: Unassigned » lucassc

Hi! I'll review.

lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented
Assigned: lucassc » Unassigned
Status: Needs review » Reviewed & tested by the community

I applied the patch in #15 for branch 7.x-1.x and works fine, changes in #8 have been made, tests are passing and there's no coding standards problems as well.

jungle made their first commit to this issue’s fork.

  • jungle committed 8c1a227f on 7.x-1.x authored by vicheldt 
    Issue #2708763 by sophiavs, vicheldt, navneet0693, jpdippenaar:...
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Reviewed & tested by the community » Fixed

The head is green https://www.drupal.org/pift-ci-job/2596707, no idea why https://www.drupal.org/pift-ci-job/2418880 failed.

Let's go ahead.

Thanks!

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.