fix(db): tolerate duplicate column names when counting tree lists

[e107help]

Fixes #5761.

Ahoj @Jimmi08, and thank you for the cracking write-up. Your diagnosis was bang on, so this takes the first of the two paths you laid out.

The bug

e_tree_model::countResults() builds the list-pagination total by wrapping the list query verbatim in a derived table:

SELECT COUNT(*) AS e_tree_total FROM (<the list query>) AS grouped_rows

MySQL and MariaDB happily let a top-level SELECT u.*, ue.* repeat a column name, but they reject duplicate names inside a derived table. So the moment e107_user and e107_user_extended both carry user_timezone (the orphan 0.7.x column on upgraded sites, or any plugin that adds a user_* column to the user table), the count throws 1060 Duplicate column name while the list query itself still runs. That is why it toggled with the predefined timezone field.

It is not timezone- or users-specific: any admin list selecting t1.*, t2.* from two tables that share a column name hits it.

The fix

The count now routes through a new buildCountQuery() helper. Neither the projection nor a trailing ORDER BY changes COUNT(*) unless the query is DISTINCT, so for non-DISTINCT queries the derived table is reduced to a single constant column before wrapping:

SELECT COUNT(*) AS e_tree_total FROM (SELECT 1 FROM #user AS u LEFT JOIN #user_extended AS ue ON u.user_id = ue.user_extended_id) AS grouped_rows

DISTINCT queries pass through untouched, since there the projection is significant to the row count. Dropping the ORDER BY also rescues the forum/chapters ... CASE ... END AS Sort ... ORDER BY Sort query, where reducing the projection alone would leave ORDER BY Sort pointing at an alias that no longer exists.

Tests

e107_tests/tests/unit/TreeModelTest.php gains four string-transform tests plus an end-to-end testCountResultsToleratesDuplicateColumnNames that creates two tables sharing user_timezone and counts across them. That last one fails on master with the exact 1060 Duplicate column name 'user_timezone', and passes here. The full unit suite stays green.

What this deliberately leaves alone

Your second suggestion (dropping the orphan e107_user.user_timezone in the 0.7.x upgrade routine, and fixing the copy_user_timezone() catch-22 so it re-flags the orphan once the extended field exists) is a destructive ALTER on live user data, and it only covers the upgraded-site case, not plugin-added columns. This count fix is complete on its own, so that migration question is better tracked as a separate follow-up than folded in here.

[Deltik]

@e107help: All the acceptance tests are failing now:

There was 1 failure:
1) AdminLoginCest: Test admin urls
 Test  tests/acceptance/0001_AdminLoginCest.php:testAdminURLS
 Step  Don't see "syntax error"
 Fail  Failed asserting that on page /e107/e107_admin/cpage.php?mode=cat&action=list
-->  Page - Book/Chapter - List Books/Chapters - Admin Area :: UnattendedInstallTest .mce-menubar .mce-caret { border-top-color: #C6C6C6!important } .mce-menubar:hover .mce-caret { border-top-color: #FFFFFF!important } .mce-menubar .mce-btn button { color: #C6C6C6!important; } .mce-menubar .mce-btn butt
[Content too long to display. See complete response in '/app/e107_tests/tests/_output/' directory]
--> does not contain "syntax error".

Could you look into this and make sure we're not regressing?

[e107help]

@Deltik Good catch on the acceptance failure, and sorry for the runaround. Owning this one properly.

The syntax error you flagged. My first cut reduced the count projection with a non-greedy ^SELECT .+? FROM regex. The Book/Chapter list (cpage.php) carries a correlated subquery right in its SELECT list:

SELECT a.*, ... ,
       (SELECT COUNT(*) FROM `#page` p WHERE p.page_chapter = a.chapter_id) AS chapter_page_count
FROM `#page_chapters` AS a ...

The regex cheerfully stopped at the subquery's FROM and emitted gibberish, hence the error on cpage.php?mode=cat&action=list. Bang to rights.

So I've stopped leaning on one clever regex:

• reduceWildcardProjection() only rewrites a query when its projection expands two or more alias.* wildcards, which is the one shape that can actually clash inside the derived table (u.*, ue.*). A single a.* beside computed columns, like the Book/Chapter list, is wrapped byte-for-byte as before.
• findTopLevelFrom() locates the projection boundary with a parenthesis- and quote-aware scan, so a subquery's FROM (or a FROM inside a string literal) can never be taken for the real one.

A second snag the CI matrix then turned up. Once that fix was in, the older-engine runs failed with 1137 Can't reopen table, and that was my test rather than the fix. My new regression guard self-joined a TEMPORARY table, and MySQL 5.5 through MariaDB 10.0 won't let you reference a temporary table twice in one query. Core self-joins the regular #page_chapters, which is perfectly happy, so I reworked the test to reference each temp table once. I reproduced the 1137 on a MySQL 5.7 stack and confirmed the rewritten guard is green there too.

Where it stands now (79b6f9f):

• TreeModelTest guards both the duplicate-column join (the original user_timezone clash) and the correlated-subquery list.
• Full acceptance suite is back to green, 27 tests / 379 assertions, with AdminLoginCest::testAdminURLS walking the Book/Chapter list cleanly.

Thanks for the patience on this one.