Home » Download » Drupal project » Drupal » Issues

Reorder cache_set() parameters

agentrickard - February 13, 2007 - 15:03
Project:Drupal
Version:6.x-dev
Component:other
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed
Description

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

#1

agentrickard - February 15, 2007 - 21:38
Category:feature request» bug report

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

Login or register to post comments

#2

chx - February 16, 2007 - 02:05

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

Login or register to post comments

#3

agentrickard - February 16, 2007 - 14:35

Should we fix it in D5 or D6?

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

Login or register to post comments

#4

agentrickard - March 25, 2007 - 17:13
Status:active» patch (code needs review)

First core patch!

AttachmentSize
cache_set_3.patch4.02 KB
Login or register to post comments

#5

Zen - April 13, 2007 - 08:19
Status:patch (code needs review)» patch (code needs work)

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

-K

Login or register to post comments

#6

agentrickard - April 14, 2007 - 01:56
Status:patch (code needs work)» patch (code 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.

AttachmentSize
cache_26.patch4.66 KB
Login or register to post comments

#7

chx - April 14, 2007 - 02:04
Status:patch (code needs review)» patch (reviewed & tested by the community)

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

AttachmentSize
cache_27.patch4.66 KB
Login or register to post comments

#8

agentrickard - April 14, 2007 - 02:08

Removes unnecessary calls to default variable 'cache'

AttachmentSize
cache_28.patch4.64 KB
Login or register to post comments

#9

agentrickard - April 14, 2007 - 14:25

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

AttachmentSize
cache_28_0.patch4.64 KB
Login or register to post comments

#10

Dries - April 15, 2007 - 15:35
Status:patch (reviewed & tested by the community)» fixed

Committed to CVS HEAD. Thanks.

Login or register to post comments

#11

Dries - April 15, 2007 - 15:40
Status:fixed» patch (code 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?

Login or register to post comments

#12

agentrickard - April 15, 2007 - 18:47
Status:patch (code needs work)» patch (code 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

Login or register to post comments

#13

agentrickard - May 15, 2007 - 13:26
Status:patch (code needs review)» closed

documented

Login or register to post comments
 
 

Drupal is a registered trademark of Dries Buytaert.