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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agentrickard’s picture

Category: feature » bug

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

chx’s picture

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

agentrickard’s picture

Should we fix it in D5 or D6?

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

agentrickard’s picture

Status: Active » Needs review
FileSize
4.02 KB

First core patch!

Zen’s picture

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

agentrickard’s picture

Status: Needs work » Needs review
FileSize
4.66 KB

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.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.66 KB

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

agentrickard’s picture

FileSize
4.64 KB

Removes unnecessary calls to default variable 'cache'

agentrickard’s picture

FileSize
4.64 KB

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Dries’s picture

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?

agentrickard’s picture

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

agentrickard’s picture

Status: Needs review » Closed (fixed)

documented