I am just trying resolve a random bug so decided to run some modules through the Coder module to eliminate that as a possible issue. Saw few issues with the D7 version that would be great to get into an upcoming release.

CommentFileSizeAuthor
#7 entity_coder_issues.patch5.6 KBfago
#2 entity-CoderReview-1078792-2.patch8.95 KBmgifford

Comments

fago’s picture

fago
Primary language German
Location Vienna
commented

Sounds good.

mgifford’s picture

mgifford
he/him
Primary language English
commented
StatusFileSize
new entity-CoderReview-1078792-2.patch8.95 KB

Well, it addresses the performance time() issues and spacing.

I've tried to fiddle with properly addressing:
hook_access removed in favor of hook_node_access. http://drupal.org/node/224333#hook_node_access

But really don't understand the module well enough. Those bits might need to be yanked out.

Hopefully this helps move it along though.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Active » Needs review

Changing status for bot.

hass’s picture

hass commented

Looks codewise all good.

mgifford’s picture

mgifford
he/him
Primary language English
commented

Would be good to have folks closer to this project verify that this is appropriate:

-function entity_access($op, $entity_type, $entity = NULL, $account = NULL) {
+function entity_node_access($op, $entity_type, $entity = NULL, $account = NULL) {
-function entity_metadata_node_revision_access($op, $name, $entity = NULL, $account = NULL) {
+function entity_metadata_node_revision_node_access($op, $name, $entity = NULL, $account = NULL) {

Status: Needs review » Needs work

The last submitted patch, entity-CoderReview-1078792-2.patch, failed testing.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new entity_coder_issues.patch5.6 KB

Thanks. Those functions are mostly not related to node_access - I'm not sure why coder suggests renaming those? I don't think they should be renamed.

+++ b/includes/entity.wrapper.inc
@@ -512,8 +512,8 @@ class EntityStructureWrapper extends EntityMetadataWrapper implements IteratorAg
-      if ($entity instanceof EntityDrupalWrapper && !$entity->entityAccess('update', $account)) {
-        return FALSE;
+      if ($entity instanceof EntityDrupalWrapper && !($access = $entity->entityAccess('update', $account))) {
+        return $access;
       }

Also, I'm not sure why the new hunk should be better.

I've re-rolled the patch without the mentioned hunks.

+++ b/entity.module
@@ -421,8 +421,9 @@ function entity_view($entity_type, $entities, $view_mode = 'full', $langcode = N
  * @see entity_type_supports()
+ * Implements hook_node_access().
  */
-function entity_access($op, $entity_type, $entity = NULL, $account = NULL) {
+function entity_node_access($op, $entity_type, $entity = NULL, $account = NULL) {

Oh no, it doesn't. It's fine as it was.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Fixed

all green, committed :)

mgifford’s picture

mgifford
he/him
Primary language English
commented

Thanks @fago. Coder definitely needs work.

Status: Fixed » Closed (fixed)

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