From 64bdc6424ca4eac4ad26b24e257c3639bbec29dd Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Wed, 21 Feb 2024 22:32:30 -0800 Subject: [PATCH] Incorrect SUMPRODUCT Calculation Fix #3909. SUMPRODUCT is mishandling multi-row ranges. In Calculation/Calculation, `checkMatrixOperands` will often resize its operands. When it does so, it needs to recalculate the dimensions of each. This fixes the reported problem. Likely cause was PR #3260. That ticket noted the poor coverage of the code being replaced. Tests of the problem in this ticket were absent and are now added. Despite this, I note that `resizeMatricesShrink` is virtually uncovered, and `resizeMatricesExpand` has substantial gaps in its coverage. I have covered some, but not all, of the Expand gaps. I am struggling to come up with examples to fill its remaining gaps and those for Shrink. However, I will merge this fix in about a week even if I don't succeed. --- .../Calculation/Calculation.php | 2 ++ .../Functions/MathTrig/SumProduct2Test.php | 34 ++++++++++++++++++ tests/data/Reader/XLSX/issue.3909b.xlsx | Bin 0 -> 10593 bytes 3 files changed, 36 insertions(+) create mode 100644 tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SumProduct2Test.php create mode 100644 tests/data/Reader/XLSX/issue.3909b.xlsx diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index e6e6921549..6028a9d72f 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -3729,6 +3729,8 @@ private static function checkMatrixOperands(mixed &$operand1, mixed &$operand2, // Given two matrices of (potentially) unequal size, convert the larger in each dimension to match the smaller self::resizeMatricesShrink($operand1, $operand2, $matrix1Rows, $matrix1Columns, $matrix2Rows, $matrix2Columns); } + [$matrix1Rows, $matrix1Columns] = self::getMatrixDimensions($operand1); + [$matrix2Rows, $matrix2Columns] = self::getMatrixDimensions($operand2); return [$matrix1Rows, $matrix1Columns, $matrix2Rows, $matrix2Columns]; } diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SumProduct2Test.php b/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SumProduct2Test.php new file mode 100644 index 0000000000..0d40b5dd92 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SumProduct2Test.php @@ -0,0 +1,34 @@ +load($file); + $sheet = $spreadsheet->getActiveSheet(); + self::assertSame('=SUMPRODUCT(((calNames=I3)*(calTiers=$K$2))*calHours)', $sheet->getCell('K3')->getValue()); + self::assertSame(40, $sheet->getCell('K3')->getCalculatedValue()); + self::assertSame(4, $sheet->getCell('L3')->getCalculatedValue()); + self::assertSame(40, $sheet->getCell('M3')->getCalculatedValue()); + self::assertSame(4, $sheet->getCell('N3')->getCalculatedValue()); + self::assertSame(40, $sheet->getCell('K4')->getCalculatedValue()); + self::assertSame(0, $sheet->getCell('L4')->getCalculatedValue()); + self::assertSame(40, $sheet->getCell('M4')->getCalculatedValue()); + self::assertSame(0, $sheet->getCell('N4')->getCalculatedValue()); + self::assertSame(24, $sheet->getCell('K5')->getCalculatedValue()); + self::assertSame(0, $sheet->getCell('L5')->getCalculatedValue()); + self::assertSame(24, $sheet->getCell('M5')->getCalculatedValue()); + self::assertSame(0, $sheet->getCell('N5')->getCalculatedValue()); + self::assertSame('=SUMPRODUCT(calHours*((calNames=I3)*(calTiers=$K$2)))', $sheet->getCell('I14')->getValue()); + self::assertSame(40, $sheet->getCell('I14')->getCalculatedValue()); + $spreadsheet->disconnectWorksheets(); + } +} diff --git a/tests/data/Reader/XLSX/issue.3909b.xlsx b/tests/data/Reader/XLSX/issue.3909b.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..8b16d5610c5bd15412fb57f3bf73d2b0f752ea89 GIT binary patch literal 10593 zcmeHN1yfvE(;h6iLvRi5?iSqLgS)%i;1(o7gF7U+YjF4A?(Py?!Z(}Uw{|zX-!FLI zJ5~4At(vE&=A3@IpYA@2G7yj$fL8z*002M&*a1LNd4K@`@lXH&IsgXzort}ii>aN9 zzN)8#sk1JFhpi29E+jZrHUJ#-{r_G6#Vb&oprp{ngwnqz_k<*)k4SrG5M_Ao|4q_3&YRfB+a4-{dBErsqjml2^Go}x7bm{GReTw9&(MM}!e!gQH<}{a7OI}5 z<0^OBYDoijZ1XCVcTq-s&0n#{>Isee7C2t#l3lthi>nzSGP@JfpN;5#4!C2d%ZIKD zh8jBZdl9EGho!&%Jb{w8!9QkbO65I&zDFP>gf7Oh0L7sf0yvW^@c*P*UiM>Sa*204 zJ;lgK>)xXmTbR(*!p)C!ey`QRQn-LQO*q^9GmeN|)RTkZ{pm7`AiY!Ylqj+>9A zYDb%b{T~v+-Np9+Yjy9qtjN9zofB!CSyrqBW_Ns+8f>UF4h=e`2HIst+Z3(qp0ErK}Y~w{ho%FR`{Y12S{!<*-GD|Ver1GcP|S~esFY!rlD|3 z5_c@!=s|Uxxt_U6k&yPJa%+vHDQWziBQv=6Mttf*v7pg4?pv zDAS3D)KlNYrsKP4T08o)550W)fHEl?+8xWdL_c}62pVnv;xL<()wE(#ts3LMgQqU^{a#o8^tX&TVc_&&|k|xmhMNR#v@O zIiHGNO>z3?JNz$j^zk&NJ65@Vx;?3RGQNlsNQ~zItVru)M>OSfU*X6<1!P)KW)L%1YbK0Uye+f)lO6)-On~XPgGn zcuL|5vkUfxxJDxY53YT)uCeN5BI|HnS*dkNs=c_xgj!=gGVsMTVw<|*Zr;G`mWgwIiMVXL%yba<&v1VGqqiR+RgKEKL@H zpb&P)gS7H;TLq4JdTrH$suS>%Ttho>NADT>B$7C#>#Zjoc(I=&+ZaZqw=(Z2-gaiS z%N4`U%3+>*l$K<+q-4-&xZelk8)ndWLXK%ET2EURocWnz`oiQW)#i~P>T76$J)7Rd zY2M&oOU~LO14L5tJV9RCgB+w&vCWxQih5c^u_5(J^43A&%2)F*1H_VZY#sUU9%UD3 zAo{SQKP2#RlGQ{!L>_;u@}-Q6F@db1Wox(zvRi*o>-uy};_xlWXt*^0iwz=x`_oA_S{$$(SFdCW>8hb--DW%E z*>6{GAA0s>eX)zM+2|lLtZ|j2cOpko@E|hujsctPd7~=yihZ?-@G@+kWD+v)cxu%nFj(;Nag-j&;|DCn?6v9|+Uwqu9F^&SX#ZRsV+-12QhFF0l^U~)0o*Mu zY^@7CEu0^hLCF<)4XX*^uNWviDeIgSVlM1l+Z&HxyRq;#hkU)4-SypjwhwJU@s)?h z?RSSj>}H&Me-u!!F|oNJKxlQW56uc}Y!%FU1Ty3bKD@aiqENMmXXsZi^kR%ZSObyx zpEQqh{8ott3IP2es^bD+z(6$rJstj)?EguPU?2?(`tE=CRT@7o-NS^^cNTbt;?!WN zp)1O<5=463`F<7}JI+$2UQU|j$H5X_iCs&j3u7zfF5abYJ3{>EO^NM1^n)TVt!Gkx zdXAd>Tz%YAnMZT;79ENvVss6aXKaA|_u==W@Ek%H%1o|Unl4puokXTr;87DgUkh*Y z)K;nRtbzAVYUD!Hg~9dG$jx;r@O$`e54vePwNK5ZWBOvnk9)UNmtqR=T(9O9prev1 zuJu4yg8PJ!-P>n~iZjUfS0qIhalOW$3R6Zf=ji`HKX`udB!Df7%Z;RrB4Ym9td%S( zP@Et8^9hXehg}M%mVM?BQ%-YU<+5`18U1TN+7^dl#F=gc7nwdjhO{LV5*07$~f6 z>8|u$S*`WB#W>WUw{P0buJ~b%7lph zwO}j#@Th^wr@YB`W>qc=P0k;Xr|>>Z;d@ZkyE$-=z~HgO+Uqt(Z0B64QC$nK{c{=P z(p|!2w{9K#fVnM_GEFowmBOau{S?J{Z|p+Le1G#dZn!+aP))_1$rwhnMk9Y58Dm{r z9M>9aY>cyeUJHp(F*9TggZB|3C&Hs1D=*zfMFdpc^ zQ4+|e+t<0bvnBIkJkBJwV1ms1%a4iPuSp)=L9|9W)CVjKZN1c{@9(j*HDIA{%PBG| zfPvr}U}8qnp+4o&3rKhx^N}bAeTWco@QkwK4a}I89UmHmcl?lY#u$pxui3U*z?Uj^$x}4$I zX}q$9$y&Pk!e=LQ=VrSW*?$6?)n7bhU7}EO$hTY}ynXrf7P@bp*%1DX$jc#xRZuF- z#AuZR7B{6;q6|p`hLZ%zEym>lH+vfiVd8AjCi_Nj{s;{DdM)@d&XWDSYDAtPi~t?5gMEl1rZ6hx5TD} zU3lP2w0G^No6Nq=l3keUmWeZ)jdstIqfqhqadVSthOQz|#W*D-aZb(P5b1coqT{0l zCI%(XT%4WT3>3Uo$5d8b$H;?g{~S4mKO%8q=m|YhxBKnNF-m?Eb55|Xy_2go(jag4=MHN1q))q4#fk~V@0{h-(s#{**nXNh z*{QUCvfAR-&=j;RXUfZQwJ4Ify{MASU)hv`&i0!DE0%C&1)H(_ES|LqUWA?e>5!^e zD4|%zwXyg^E_jiqT97IyQ9e(2xmwt28|#^>uA%Lhn5ict6kgG}V~b60_@1!|I_L=@ z7r7v_3$v6dV&+pgYiF_;ANFwkTDtsSRX zGSE zgxn$t?AihwW{^0ajTVkP5JTX$ciUG4e>&#Rn@RLrq%L9R_84Zr9#@wHF@>kWWw!z!RjjbPSvIp*OjtcWdUhRt2`rjhbE`CRdrc z1$ZTQGhs?@CvUqsiw=l{yTXr`@DuB31@}vTT+X-*yMf8nqM_S6?7dea{W#D0^XGpp zF{{K7k9D8{0C1AudO|+h(}DWbfI zvh^@wblR3oNF{VrA^w5`9}v*Ny@N(9}|xZa z5UwanbxKrmcOa+OgsA0(PFfeDjrO3uk5*rdb2 z#Q!CQyZ6b1)e>9WFt2689w2MG6*_8YJU_qB3gAoWC}w@9lQF6-I8ybR>T!R9-va4S z4ch*o!hT^n`l=#`Gg}te7(x{Nur3B|FN7no*d@{GX1{*OnT@bE-^EG0@^;?k3Quvw z!mB!@Tr}DX*t&>l(znotwHni)5i-`1oJeTOVKlkOe(wc2nFDjb(n$*IA9gv-(xJ0mhKN$|b-r~xg!E|C0h}mj4f)C9v!1c0)9_JJD@g+T z6<{t(PgSG|+6gBk*5k%89ujg13-ywt4yxiML%s>f5i`(c1)x~W^+XSKjC;a*9GX<9z~@%$sjdNXyzRE7F8tr8C8Zs*n~a>}L93Yrk!W zK-fZlBzZ^9piJepIpyik-S82@Z!*@g`g~D#YQE2pRDoEGxp_Ph*>7NlAUBHyiE9Pt zR3d?E!;c@857Mbljs{H>Sc8$Nxk|dc5=+a`-P6ya`;qH-5pz0?pmk?i>I6aD(;a;Fa8!vE;cK6IYGn(eXTzg(G0@H(iJLVR=fo2~Q`z1pCGo3bG0W^)ORApxsJOVP*eX!a zSFOgHQGbR-qi#fve#`sOi?L2tsZGdK#l=VVg|bQU(OevdSD98EDgOW<<9raz}~jQ?_1; z!`mMeYLG1Iid!;Lc*)g!J;)cMMEa#o_r+`W4{fK@1P+#6jOJchEO0V+rH-)Yo-*e) z5eA1pWulsyCL4(@8H>r3=!aPm^hVQm)XJ%@U*mPkcvFUVmDV6cb5qR}@yKh%dlKjp zq{~EKF_^RPLS!%NnG2yF)YrT2U0xkltF{Q`6TB&PagMH>WC~5*=|1rBZe z3~)RxF%1h#vUjkI;oC`48V3c_kfP*CQGOd)Xo_W}`my)|!@ioTIfnD7n@-!VR+x6Wp0Llo&Gk}t}@le#X~ zu5~&(*UJ@KjW$T{gc_fYM!s0dtyM7a2%&z6@qYAfRt#9(R@KC@A(fK${;8UIS?5Z_ z8mnbr0t05hM%;F~yumwKxn-3gvFmMxn$;~{>tyzXJ(75k-?UY1=Bxc_|8BchB9729 z<-EnS&EiobZ5>*0Gu1<&Cw&-AOu;f%Apy~~UcnvBd=_!R> znaRkVd5+g47UVZ`yBSVo=#-}?NRWH;HOeq7#-nz$O}Uz3n2n03F*VW*^;g6sggza#(44yu-Ux_){c;*W%SW%eDy|F7*CZcybmIixyT! z)2UnJ8MnRsamnKEbX(v%$eYVO2lXa1;<LwW?fGBJ!Rn(2G{RgPyu-K-IjcJ& zW&yolouJA+6O_6V(O~QzGoy%E9@hdZAIlQ`}AxhCL80DkHU@$p_6)3 zHpn^Cw^o6P5;?EGu5TcAs?5T{|CSxcVot;{zr547h} zRa4QMBv~MD#V)7PAh?Z}8dKNJa>R&brqmX=P;JXBtAGflJ*qZtk-m865*p91wv~{< z{nQ|oWP6|wT7iEC2hvMyj(X7g(103XfAqO+OYU!@3-Em=ffe^Y z>Q140uS}oHch@pGdgE#mI%z9AjYp9`-N}8w5ZPf03Pgy4^eOgCb8bK*9ehoYueJYe zDh6rdZ6V94=pkvKG`*3Vx=zAQfr9~G7KXYv-V85}xd{UGK3{xiM0VZBJ9!H~^4yOn z>$_xy>+N;ZoeyR@yXl@YZ$?M4BQTNrwx4(}n8 zNt1gtLQk2fDv^}?>grx1%`5rQWEWkq-SZ`ZUur1qWo3=^Uw4l$84$h8E7H~uZcO2d z-RlooTE~CjvwSOU#u?>q5u+sEj8^t6tb#u)rV_Cw+Mg{~@VG32j}&F>M{thNeIuc= zA+T1&=ch#^Hwv_!acfDCevP}U>C@cS1D=#s5gDShFGfB5-n!{d4&@e39T-pny^j1* zPktm+@uP6H8X!{^gV~+j&*(fm7&%6 zGQIln*tPbf<#vPC%|MX>z%=I*=K04p2s~jJgDum9mp2L;-G(1I-H#~9VzIx_Y7;Ok zH@qX>CSX<<{2;Z6xV2pCUSn8V*owEJ6LN z8!I~5J2*2M+dG;5ws4?n>Hk{7Acu;IQ&I?G!UzG4^^x<>6ybQxA?TE9is*5BEqqE& zR>bo|YSsGM8#+Si9@dS|xSzeo7=ZZ3Yi1Rk{~Ii#ppWMOVA$IRga9hcI;Qdr^f4SsvQxt3`O7dNF;^%W8I?5?EX=ut zZW&09MK+z#jxJ|%%(!WVu&Ir)8<*%$&3gRTYR|&1Et~$;q7FmsOz#ekQH4D2W9qWe z`BL0@=~Ak!8&2iGHVa2j@f26#BHaf;4S*x0r3UPIq$8(^oL(i*2>Y{3@}XR&=JW0W z$~o>76|{-F!!qw5+kVN;*r2uw5amr=JWjMRuAY3jgwX7sz%$;$zc*9D@T;hrKz_LQ zlfQrQLqi9L|G6Q^|NgkrRpjhJQnnR&9?0Zkzip$%3|SpXvhtx@AaL;*egWmp!+8!VWC3h*)_R?MI+Koi*z1s=)E1O$(ya_{ugrkjF^WmUB^SoN(`dra%OE3IqG)o6I};P3ZhtC+|Iph zVps0{AWo0CFYDh`x3$?TNsP3X;3%-pn1KX7NcHtu&t0tQYGQfU8vqU=B0 z`=95(oG(+9`MZI?pK$pP@V93bh(mul@A50~*Qx)X&{