Please provide test instructions

Since JHelperMediaTest does not test this method and i don't know how to write a Unit test I adapted from two other methods in this class which are testing static methods hoping i did it right.

He's asking how you can verify the change actually works as intended. Not that the unit test is needed.

So, how can one validate this patch and what purpose does this patch actually serve to improving our code base? (The latter is addressed in your description)

@mbabker I edited my previous post while you wrote yours. Would you mind to review what i added and suggest what to change or improve, please?

A static method is bad for testing, so we are following the opposite way and replacing static methods with non static methods. Tbh, chances are not good to get that merged.

This change covers only 7 lines of code at all in whole Joomla directory - 6 of them in two controllers and 1 in the helper class. How big is the risk to break something through it? I mean, we are talking about a number conversion and there was no test for it at all before this change.
If static methods are bad for testing how do you test all the static methods in all these core helper classes?

We are seriously struggling to unit test a lot of the pure statically created classes. Hence why we are slowly moving them to OOP as we refactor and can break b/c in major versions

What does b/c mean?

It changes the behavior of the method and the API design. Static methods can only reference static containers and cleaning those up is a huge pain (you seen all the hacks dealing with the static JFactory in the test suite?). Object oriented classes/methods on the other hand are easier to inject dependencies into.

So, this gets into an edge case. The method itself doesn't have any external dependencies so could be fine as a static. But, if something changed later about it to add in some sort of dependency (like an application parameter to force a size), then the method would probably have a static call to a static method or container and we're back in testing hell.

What does b/c mean?

backwards compatibility

B/c = backwards compatible. Sorry for not writing it in full!

@rdeutz, @wilsonge thank you!

@mbabker No, i have not seen all the hacks dealing with the static JFactory. I get your point. One thing i ask now is: Is testability more important than performance (memory consumption)? If i got you corrently than i think the helper should be added a getInstance() method. This would allow for dependency injection, instance creation and one-line calls to utility methods like JHelperMedia::getInstance()->toBytes(123). Please correct me if i'm wrong.

That's a slippery slope too. A lot of our getInstance() methods revolve around singleton objects (like those in JFactory). If it were spinning off a new instance each time, it could work, but then why not just do (new JHelperMedia)->toBytes(123); (OK, that's PHP 5.4+ only, but same thing in theory as the getInstance() call without a singleton object).

Testability and performance are both important. But, there needs to be some precedence to fixing the application's testability (which means fixing behaviors that were once accepted) too in order to keep providing a quality product.

Think i got it. It wasn't clear to me that there's such a big code refactoring going on in the background. If i had know about that i wouldn't have suggested this change. I'm gonna close this PR.

@mbabker Thanks for mentioning (new JHelperMedia)->toBytes(123);. I havent seen this before. Is this an anonymous function call? Nice!

It's a PHP 5.4 feature. http://stackoverflow.com/questions/2188629/how-to-chain-method-on-a-newly-created-object explains it better than the official release docs.

Thank you all.