This would need a lot of testing since you're sometimes now are checking variables stricter than before.
A lot of the changes also look very pointless to me, like changing " " to ' ' or changing is_null to a comparision === null.
Personally I'm not in favor of doing such changes because while they might have a very tiny effect on performance, in practice you will not notice them at all. Even after 1000 instances of them.

On the other hand the chances that some bug slips in is higher than the tiny gain we get.

As long as we don't have a codestyle rule regarding those cases (which I would not be in favor of), I wouldn't merge this.

@frankmayer there are a lot of changes and you put a lot of work into it, this is much appreciated, but I have to agree with @Bakual the chance to break something somewhere is much bigger then the performance advantage we can get. If we would have proper unit tests to make such refactoring safe I would merge it but we don't so I also wouldn't merge it.

There is a lot of work of this kind to be done in Joomla4 and I suggest to contact @mbabker or @wilsonge if you can help them.

If we would have proper unit tests to make such refactoring safe I would merge it but we don't so I also wouldn't merge it.

Then we can't ever merge anything like this outside the libraries folder unfortunately. Modules aren't even (practically) testable because of their procedural nature; you basically include the entry file and hope for the best.

Ya it's a lot of stuff to go through, but whether it be the maintenance team on the 3.x branch or the few of us really focusing on the 4.0 branch someone's gotta do it at some point. And admittedly things are going to be much better tested on the 3.x branch until 4.0's somewhere around a beta.

In 4 we have to do a lot of cleaning up the house stuff, I am saying spend time there it makes much more sense. If it is $var == 'alias' or $var === 'alias" doesn't makes a difference

@Bakual @rdeutz
I understand your points.
As for the changes made, they are more or less based on best practices.
For example:

• It's better to use single quotes than double quotes if not embedding some variable within the string .
• is_null() is a function call and thus more expensive than just comparing to null
• The order of conditions in if statements also makes a big difference, if the result of the condition can be computed at the beginning with cheaper instructions, than the ones where multiple functions may be called.

As for the strict typing, I hand picked and checked those one by one. At points where it was unclear, I didn't change them. So most of the times strict typing was enforced when there really were strings to compare to or where a function involved in the comparison would return a string or definitely an integer.
So, a lot of work has been done in those PR's with which anyone having to deal with strict typing in future, will not have to deal with.

As for the micro-optimizations and if they are noticeable or even necessary, that is a big discussion. My personal opinion is that whatever causes a machine to do less work, is acceptable.

@rdeutz actually it does make a difference, unless you don't want strict typing implemented, which is also a valid way to look at things. However IMHO strict typing makes things a lot clearer and easier to understand. Loose typing is always a bit of unclear and error prone.

@frankmayer I don't disagree with the best practices. For new code they may even make sense to a degree. But going back and refactor existing code for such microoptimisation is a waste of time of the ones writing them, the reviewers, testers, mergers and finally the other PR writer who need to solve the conflicts this PRs are going to generate.

As for the micro-optimizations and if they are noticeable or even necessary, that is a big discussion. My personal opinion is that whatever causes a machine to do less work, is acceptable.

That's where we disagree. I can support microoptimisations for code that is run hundreds of times in a request cycle. Eg stuff related to build and parse URLs in Joomla or library stuff like JRegistry calls.
But here we are talking about modules, they are run once per cycle and most of the time even are cached. It's a waste of human ressources here.
Same if we're going to talk about administration area, there performance doesn't matter a lot as well.

So it really depends on what you are touching if it is worth or not.

is_null() is a function call and thus more expensive than just comparing to null

Btw, I read some comments that in PHP7 is_null is actually faster than === null. Haven't tested it (not worth my time ?)

@Bakual I can understand your human resources point micro-optimizations but not for the best practices. For best practices such changes are building blocks for future updates using more compliant code (which is less error prone, etc...).
As for where best practices or even optimizations are done, it does matter. For code compliance inside a project, all parts should be on the same level with best practices or use optimized code.
According to Joomla's stats here Joomla runs on at least tenths of thousands installations. So, once you're in that ballpark, those micro-optimizations do matter when thinking in terms of wasted energy because of non-optimal code. But that's just my opinion.

BTW, those PR didn't cost me but a few hours. I just wanted a change of scenery and thought of giving back to the project. A bit of cleaning up and optimizing stuff was my idea to do that. Some super-optimizations might be an OCD of mine, but since I was already at it, I just did those, too.

If you think this PR is not worth the time, then by all means throw it away.

As for the is_null() being faster, that's a really nice development. Things keep changing... ?

I have tested this item ✅ successfully on 933ab8d

on code review

The was a small hiccup with the last push, which was resolved. Should be OK now.

I have tested this item ✅ successfully on 72e4979

code review

Conflicts resolved...