Download & Extend
Issues

Reorder cache_set() parameters

Posted by agentrickard on February 13, 2007 at 3:03pm
Project:Drupal core
Version:6.x-dev
Component:other
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

The parameter order of cache_set() does not follow Drupal's coding standards. Ref: http://drupal.org/node/318.

Arguments with default values go at the end of the argument list. Always attempt to return a meaningful value from a function if one is appropriate.

http://api.drupal.org/api/5/function/cache_set

cache_set($cid, $table = 'cache', $data, $expire = CACHE_PERMANENT, $headers = NULL)

Specifically, since $data is not default, it should occur before $table.

cache_set($cid, $data, $table = 'cache', $expire = CACHE_PERMANENT, $headers = NULL)

Writing the function the second way also allows porting of 4.x.x modules to 5.x and higher without rewriting all cache functions.

Login or register to post comments

Comments

#1

Posted by agentrickard on February 15, 2007 at 9:38pm
Category:feature request» bug report

I think this is actually a bug, not a feature request, so refiling.

#2

Posted by chx on February 16, 2007 at 2:05am

Yes this is somewhat a bug because it's not Drupal coding rules but PHP rules...

#3

Posted by agentrickard on February 16, 2007 at 2:35pm

Should we fix it in D5 or D6?

If D6, I'll take a stab at it during DrupalCon, if not before.

#4

Posted by agentrickard on March 25, 2007 at 5:13pm
Status:active» needs review

First core patch!

AttachmentSizeStatusTest resultOperations
cache_set_3.patch4.02 KBIgnored: Check issue status.NoneNone

#5

Posted by Zen on April 13, 2007 at 8:19am
Status:needs review» needs work

Patch fails in locale.module and is missing a newly introduced cache_set call in theme.inc. Please update.

-K

#6

Posted by agentrickard on April 14, 2007 at 1:56am
Status:needs work» needs review

Well, when it takes 3 weeks to review a core patch to a primary function, sometimes core changes.

This needs to get in ASAP, before any more changes are made to the cache system.

AttachmentSizeStatusTest resultOperations
cache_26.patch4.66 KBIgnored: Check issue status.NoneNone

#7

Posted by chx on April 14, 2007 at 2:04am
Status:needs review» reviewed & tested by the community

Right. The patch is rerolled because patch was complaining a bit about it but not touched in any way.

AttachmentSizeStatusTest resultOperations
cache_27.patch4.66 KBIgnored: Check issue status.NoneNone

#8

Posted by agentrickard on April 14, 2007 at 2:08am

Removes unnecessary calls to default variable 'cache'

AttachmentSizeStatusTest resultOperations
cache_28.patch4.64 KBIgnored: Check issue status.NoneNone

#9

Posted by agentrickard on April 14, 2007 at 2:25pm

There may be an end-of-line problem with patch 28. This one tests correctly.

AttachmentSizeStatusTest resultOperations
cache_28_0.patch4.64 KBIgnored: Check issue status.NoneNone

#10

Posted by Dries on April 15, 2007 at 3:35pm
Status:reviewed & tested by the community» fixed

Committed to CVS HEAD. Thanks.

#11

Posted by Dries on April 15, 2007 at 3:40pm
Status:fixed» needs work

I'm marking this 'code needs work' until the upgrade instructions in the handbook have been properly updated. Can you take a look at that agentrickard?

#12

Posted by agentrickard on April 15, 2007 at 6:47pm
Status:needs work» needs review

I don't have edit permissions for the parent (http://drupal.org/node/114774), so I made a child page.

http://drupal.org/node/136533

#13

Posted by agentrickard on May 15, 2007 at 1:26pm
Status:needs review» closed (fixed)

documented

nobody click here