Page MenuHomePhabricator

Modified files not counted in total when attaching files
Closed, ResolvedPublicBUG REPORT

Description

On line 1124 https://phabricator.wikimedia.org/source/phabricator/browse/wmf%252Fstable/src//applications/transactions/storage/PhabricatorApplicationTransaction.php$1124
the total number (parameter number 2 of the line 1121) adds parameters count($add) (parameter number 3 of the line 1121) and count($rem) (parameter number 5 of the line 1121) but forgets parameter count($mod) (parameter number 7 of the line 1121).

Event Timeline

Sorry I don't know what is "upstream". Do I need to open this issue elsewhere ?

The last link shows discussions about questions : according to my description, I don't think any discussion is needed, I don't feel the need to ask any question : the bug is clear, the solution is clear. I know php and I could fix this bug by myself (but I will not).

Aklapper renamed this task from bug on PhabricatorApplicationTransaction.php to Modified files not counted in total when attaching files.Feb 21 2025, 12:43 PM
Aklapper changed the task status from Open to Stalled.
Aklapper claimed this task.
Aklapper moved this task from Reported Upstream to Patch proposed upstream on the Upstream board.

@Mahabarata73 thanks again for your report. Can I ask how have you discovered this problem? Are you a translator? (I think yes)

Or, have you found this problem from a specific page? I ask this since I have some difficulties in reproducing this problem, that needs a single transaction with add+remove+modified files. This can be very probably caused only from a particular API request, and not from web interface. This may be a good news since it may have not affected anybody (apart from translators).

Lines 1131 to 1157: update of 2 types of files =
*According to line 1133 (and line 1131), the second parameter (number of attached files) is the sum of the number of added files (param 3) and the number of removed files (param 5): the calculation of this second parameter (line 1135) is consistent with the text of line 1133.
*According to line 1142 (and line 1140), the second parameter (number of attached files) is the sum of the number of added files (param 3) and the number of modified files (param 5): the calculation of this second parameter (line 1144) is consistent with the text of the line 1142.
*According to line 1151 (and line 1149), the second parameter (number of attached files) is the sum of the number of removed files (param 3) and the number of modified files (param 5): the calculation of this second parameter (line 1153) is consistent with the text of the line 1151.

Lines 1158 to 1175: update of 1 type of files =
According to line 1158, the second parameter of line 1160 (number of attached files) is the number of added files: the calculation of this second parameter (line 1162) is consistent with the text of the line 1160.
According to line 1164, the second parameter of line 1166 (number of attached files) is the number of removed files: the calculation of this second parameter (line 1168) is consistent with the text of the line 1160.
According to line 1170, the second parameter of line 1172 (number of attached files) is the number of modified files: the calculation of this second parameter (line 1174) is consistent with the text of the line 1160.

BUT

Lines 1119 to 1130: update of THREE types of files =
According to lines 1121 AND 1122 (and line 1119), the second parameter (number of attached files) is the sum of the number of added files (param 3), the number of removed files (param 5) AND the number of modified files (param 7) : the calculation of this second parameter (line 1124) is INCONSISTENT with the text of the lines 1121 and 1122 (the calculation of the second parameter sums only 2 parameters instead of the 3 parameters).

Thank you very much @Mahabarata73 for this detailed report, but I was just asking "how" have you discovered this and I'm still curious (so to better understand how to test this properly and quickly). Thanks for this small clarification.

BTW patch welcome 👍

Edited: as I've already said above, I ask this since I have some difficulties in reproducing this problem, that needs a single transaction with add+remove+modified file, but it's not so easy to reproduce such transaction. Where you have such situation? Thanks for this little additional help.

I found this bug only because I read the code and this code is obviously inconsistent. When this code is executed? Or is this code sometimes executed? I dont know.

I can also tell you, when reading the code of the "if-then-else" between lines 1113 and 1180, that lines 1176 to 1180 are useless : if $add=0, $mod=0 and $rem=0, that means that no files have been attached so that these lines will never be executed (which is not a problem of course so you can keep them).

Yes, the Phabricator/Phorge codebase has quite a few unreachable else fallbacks/catch-alls according to static code analysis

This issue should now be fixed on phabricator.wikimedia.org after today's software deployment in T404134.