When the parameter user_limit is 0, we dont need to call file_space_used.

CommentFileSizeAuthor
#14 197266.patch1.03 KBdrewish
#11 file_197266.patch2.01 KBdrewish
#5 197266-2.patch1.89 KBc960657
#4 197266.file_validate_size.patch1.31 KBdave reid
#2 file.inc_.quota_2.patch1.23 KBlilou
file.inc_.quota_.patch1.14 KBufku

Comments

drewish’s picture

drewish commented
Version: 6.x-dev » 7.x-dev
lilou’s picture

lilou commented
StatusFileSize
new file.inc_.quota_2.patch1.23 KB

Reroll.

drewish’s picture

drewish commented
Status: Needs review » Reviewed & tested by the community

a small change to help avoid an extra query.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 197266.file_validate_size.patch1.31 KB

Looks like you're missing a closing brace on that if statement. Also please use cvs diff -uP so we can get the relevant function name in the diff. Here's a revised patch that also fixes the PHP doc for $user_limit.

c960657’s picture

c960657 commented
Status: Needs work » Needs review
StatusFileSize
new 197266-2.patch1.89 KB

Reroll. file_validate_size() is already covered by unit tests.

Status: Needs review » Needs work

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14498. If you need help with creating patches please look at http://drupal.org/patch/create

drewish’s picture

drewish commented
Status: Needs work » Needs review

ignore DrupalTestbedBot it's buggy: #307197: Testing the incorrect patch posting multiple comments.

drewish’s picture

drewish commented
Status: Needs review » Reviewed & tested by the community

just re-ran the tests, they're still good. simple enough patch too.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 7.x-dev » 6.x-dev

Thanks, committed! Looks like this could go in 6.x too.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

Looks like a good improvement. However (1) it does not apply to Drupal 6 (2) I'd add a comment to the if() which would explain that the function call is in there to help performance a bit. Otherwise someone might go and "prettify" if later on.

$ patch -p0 < 197266-2.patch.txt 
patching file includes/file.inc
Hunk #1 FAILED at 728.
Hunk #2 succeeded at 652 (offset -94 lines).
1 out of 2 hunks FAILED -- saving rejects to file includes/file.inc.rej
drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new file_197266.patch2.01 KB

how's this look?

c960657’s picture

c960657 commented
Status: Needs review » Reviewed & tested by the community

Looks good.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Quick fix

Committed to Drupal 6. Let's get that added comment to Drupal 7 too.

drewish’s picture

drewish commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 197266.patch1.03 KB

here's that comment for D7

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

ufku’s picture

ufku commented

According to the comment
"when a limit is provided we save a query by only calling file_space_used()"

Should't we
"save a query by not calling file_space_used when there is no user limit."

I mean the current comment is incoherent.
Or is it time to go to bed for me?

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix

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

Issue tags: +Quick fix

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.