Problem/Motivation

As identified in issue #1423460: [META] List of bugs and typos in Drupal core found by Spleshka #12, hook_node_grants_alter was being passed $account, but using $user.

Proposed resolution

As this is my first attempt at addressing core API issues, patch is in first comment.

EDIT: Removed info about 2nd issue.

CommentFileSizeAuthor
#3 Define_user-1469758.patch554 byteschertzog
#3 Remove_op-1469758.patch2.99 KBchertzog
#1 Remove_op_and_define_user-1469758.patch3.08 KBchertzog
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chertzog’s picture

chertzog
he/him
Primary language English
Location Harrisburg, PA, USA
CreditAttribution: chertzog commented
Status: Active » Needs review
FileSize
Remove_op_and_define_user-1469758.patch3.08 KB

Attaching patch for review.

Crell’s picture

Crell CreditAttribution: Crell commented

Shouldn't these be two separate patches? It's confusing which part is talking about what.

chertzog’s picture

chertzog
he/him
Primary language English
Location Harrisburg, PA, USA
CreditAttribution: chertzog commented
FileSize
Remove_op-1469758.patch2.99 KB
Define_user-1469758.patch554 bytes

Since the issues overlapped one the hook in question, i thought i would put them together, plus the one was simple. But here are patches separated.

#1423460: [META] List of bugs and typos in Drupal core found by Spleshka #12 is address with Define_user-1469758.patch

The others are addressed in Remove_op-1469758.patch

Crell’s picture

Crell CreditAttribution: Crell commented

It's generally preferable to put a separate bug in a separate issue, too. Not just separate patch files.

chertzog’s picture

chertzog
he/him
Primary language English
Location Harrisburg, PA, USA
CreditAttribution: chertzog commented
Title: hook_node_grants & hook_node_grants_alter » Define $user in hook_node_grants_alter

Ok. Renaming Issue to address the first issue.

#1423460: [META] List of bugs and typos in Drupal core found by Spleshka #12 is addressed with the Define_user-1469758.patch above.

Creating new issue to address second issue.

Status: Needs review » Needs work

The last submitted patch, Remove_op-1469758.patch, failed testing.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs work » Needs review

The green patch in #3 looks good visually, but I'm surprised it managed to last this long. Shouldn't we have a unit test to confirm that? It seems the sort of bug that would have broken rather drastically before now.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: other » documentation
Status: Needs review » Fixed

RE #6 - This is an example function in node.api.php, not anything that gets executed. As such it is actually just documentation, and I went ahead and committed it to Drupal 7/8.

Crell’s picture

Crell CreditAttribution: Crell commented

Oh. Duh. That's what I get for only glancing at the patch. :-) Thanks, Jen.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Removed info about second issue.