?
avatar mbabker
mbabker
28 Aug 2018

Summary of Changes

One of the security patches for 3.8.12 adds an extra check in Joomla\CMS\Form\Form::validateField() and makes use of an optional method parameter to do this. For the single legitimate call to this method in the production code, this assumption is OK to make, but it has broken our unit tests and potentially causes issues if someone has subclassed this class and calls the method omitting the optional $input argument. Therefore, we must check the argument is provided before using it.

Testing Instructions

I think you're going to have to rely on the tests and code review for this one. There is not a use in this repo that can create this issue outside the unit test suite, and I'm not about to build an extension package to try and recreate this bug so it can be "easily" validated.

avatar mbabker mbabker - open - 28 Aug 2018
avatar brianteeman
brianteeman - comment - 28 Aug 2018

Maybe I am being daft but isn't the point of unit tests is that code changes shouldnt break the tests

avatar mbabker
mbabker - comment - 28 Aug 2018

Yep.

To be honest though the tests that failed with this change aren't great tests in the first place (they're doing some janky stuff to directly test non-public APIs versus only testing the public API which indirectly would cover this method). But, the janky tests did expose a legitimate production issue with trying to use an optional argument without checking that it was actually given in the first place, so in this case it did help with something, but in a very unexpected way.

avatar franz-wohlkoenig franz-wohlkoenig - change - 28 Aug 2018
Status New Discussion
avatar franz-wohlkoenig franz-wohlkoenig - change - 28 Aug 2018
Category Unit Tests
avatar mbabker mbabker - change - 28 Aug 2018
Status Discussion Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-08-28 23:03:37
Closed_By mbabker
Labels Added: ?
avatar mbabker mbabker - close - 28 Aug 2018
avatar mbabker mbabker - merge - 28 Aug 2018
avatar joomla-cms-bot joomla-cms-bot - change - 28 Aug 2018
Category Unit Tests Libraries

Add a Comment

Login with GitHub to post a comment