Remove unnecessary GROUP BY clause

[loekvangool]

Fixes #5206

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[Copilot]

Pull request overview

This PR removes an unnecessary GROUP BY clause from the SQL query in getProductCount() method and modernizes the method signature with type hints.

• Removes redundant GROUP BY clause from a SQL query that already filters by a single category ID
• Adds parameter type hint and return type declaration to improve type safety
• Minor docblock formatting improvement

[addison74]

There are a few PHP CS Fixer errors.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[addison74]

All green this is good.

I’ve reviewed the change together with the current implementation of Mage_Catalog_Model_Resource_Category_Flat::getProductCount().

From an SQL point of view, removing the GROUP BY clause is correct and desirable here. The query already filters by a single category_id in the WHERE clause and then performs COUNT(product_id). With that WHERE filter, there is effectively only one logical group, so the GROUP BY category_id is redundant. Both queries (with and without GROUP BY) return the same scalar result when used with fetchOne(), but dropping GROUP BY avoids an unnecessary grouping step and should be friendlier to the optimizer, especially on large catalog_category_product tables. So the core fix for the linked issue looks correct and appropriate.

The PR also tightens the method signature:

public function getProductCount(Mage_Catalog_Model_Category $category): int

Semantically, this matches how the method is intended to be used: it operates on a Mage_Catalog_Model_Category and already returns an int (it casts the fetchOne() result). However, this change can be backward‑incompatible for any third‑party code that currently calls getProductCount() with something other than a Mage_Catalog_Model_Category instance (for example, passing a raw category ID). Such calls would start throwing a TypeError in PHP 7+.

From a pure bugfix perspective, I’d suggest one of the following:

• Either keep the SQL change in this PR and consider moving the parameter/return type hints into a separate, clearly documented “add type hints” PR, or
• Explicitly note in the description that the type changes are intentional and may break non‑conforming third‑party code, so maintainers can weigh the BC impact.

In summary, the removal of the GROUP BY is correct and improves the query; the method type hints are technically correct but may warrant a separate discussion due to potential BC implications.

@colinmollenhour - please take a look to this PR.

[loekvangool]

I'm going to get chewed out for this but, sometimes it's worth it:

This is far too pedantic for my taste. I'm definitely not a position to fight all the tooling employed to defend against misplaced whitespace and curly brackets and the likes. I have diagnosed, fixed, tested, deployed and did a project-wide search for the same issue faster than I get past these 'quality'* tools. I'm sure you all mean well, but I do not use any of that stuff and so it mostly serves to hold me back.

* "Quality" meaning: The warm feeling of knowing that the characters are lined up, cased and spaced just right, such that it gives a false sense of confidence in their actual usefulness. Like a wonderfully orchestrated military parade of a country that cannot defend its own submarines in its own harbor. Or astrology. So confirmed by multiple complex QA tools that, and I'm not going to check this, themselves are an absolute mess internally because all great utilities are a mess internally.

Please, whoever reads this, take this tiny little PR and make it your own, you have my blessing!

Rant I:
Maho implemented a fix 40 minutes after the bug report, just before midnight on Jan 1st. Nothing but hate for the way that guy behaved against some of you, but you can't deny the velocity.

Rant II:
And let's face it: in this project, it's mostly putting lipstick on a pig. Unless we are willing to let Rector et al. go into the core and change almost every line of code and call it an improvement, these rules are only going to touch the very localized changes that happen to be modified this month. All other code (in the same file) passes, regardless of how much it does not comply. Which is like patching a hole in a submarine with 1000 holes in it. I've read "Oooh yes that would be very nice to have but I'm hesitant to make such a big change to such a super-duper-core file, it's probably going to break a lot of 3rd party modules", it will remain a 'pig' for the time being. First there are no @param comments, and then we had tooling that spits them out everywhere. And now, they need to be removed again. But price.phtml is still there, taunting us with its hideous code.

Happy new year, I (L) you all.

[addison74]

@loekvangool - I understand the frustration with tooling and strict “quality” rules, especially when you’ve already diagnosed, fixed, tested, and even searched the codebase for similar issues faster than it takes to satisfy all the checks. It’s clear your intention is to help the project, not to get stuck in bureaucracy.

On the other hand, for a larger open‑source project, a certain level of standardization (formatting, QA tools, coding rules) does have benefits in the long run:

1. it makes the code more predictable for future contributors,
2. reduces the risk of regressions when multiple people touch the same areas, and
3. helps newcomers orient themselves more easily in the codebase.

That doesn’t mean the current setup is perfect or free of overhead — your points about the gap between “ideal rules” and the actual state of the code are valid. Realistically, we probably need a balance between pragmatism (quickly fixing real bugs like this one) and technical discipline (tooling, broader refactors where it’s feasible and safe). I don't like it when I see tests failing either, but if Copilot is in charge of PR, I ask the AI to fix them for me. My only concern is BC, which is why I'm waiting for more informed opinions.

Happy New Year, and thanks again for the PR and for sharing your thoughts.

[sreichel]

Thanks for PR and input.

• "Quality" meaning: The warm feeling of knowing that the characters are lined up, cased and spaced just right, such that it gives a false sense of confidence in their actual usefulness. Like a wonderfully orchestrated military parade of a country that cannot defend its own submarines in its own harbor. Or astrology. So confirmed by multiple complex QA tools that, and I'm not going to check this, themselves are an absolute mess internally because all great utilities are a mess internally.

With php-cs-fixer and rector we have tools to modernize/unifom/standardize code. Why not use them?

The linedup characters ... i have edited thousands of docblocks, carying about spacing and order. Now its fixed automatically.

For rant I ... instead of creating a well described issue, you could have create a PR directly. Id not have reviewed within 40min as Jan 1 is my birthday, but it would been merged in time - without added type hints (that should be discussed for coming releases)

For rant II ...

First there are no @param comments, and then we had tooling that spits them out everywhere. And now, they need to be removed again.

Dont blame the tools :P

You have added type hints and that makes parameter docs obsolete. (pls revert)