Using `joinFields` on multiple pagetable fields causes missing some child pages

[tuomassalo]

Short description of the issue

When a page has more than one PageTable fields that are joined with joinFields, some of the c1_ref or c2_ref subpages are missing from results.

Expected behavior

Adding joinFields should not affect the result, only performance.

Actual behavior

In some cases, not all subpages are returned when joinFields is used.

Screenshots/Links that demonstrate the issue

Possible cause: using a multi-table join causes the GROUP_CONCAT result string to include the same pages multiple times. PW runs a query like this: (formatted, and replaced the \0, with ,)

SELECT
    (
        SELECT
            COUNT(*)
        FROM
            pages AS children
        WHERE
            children.parent_id = pages.id
    ) AS numChildren,
    GROUP_CONCAT (field_c1_ref.data SEPARATOR ',') AS c1_ref__data,
    GROUP_CONCAT (field_c2_ref.data SEPARATOR ',') AS c2_ref__data,
    pages.*,
    UNIX_TIMESTAMP (pages.created) AS created,
    UNIX_TIMESTAMP (pages.modified) AS modified,
    UNIX_TIMESTAMP (pages.published) AS published,
    pages_sortfields.sortfield AS sortfield
FROM
    pages
    LEFT JOIN pages_sortfields ON pages_sortfields.pages_id = pages.id
    LEFT JOIN pages_access ON (
        pages_access.pages_id = pages.id
        AND pages_access.templates_id IN (2, 3)
    )
    LEFT JOIN field_c1_ref ON field_c1_ref.pages_id = pages.id
    LEFT JOIN field_c2_ref ON field_c2_ref.pages_id = pages.id
WHERE
    (pages.id = 1761)
    AND (pages.status < 1024)
    AND pages.templates_id NOT IN (2, 3)
    AND pages_access.pages_id IS NULL
GROUP BY
    pages.id

In its c1_ref__data field, the query response has something like 1762,1762,1762,1762,1762,1762,1762,1762,1762,1762,1762,1762,1762,1762,1762,1764,1764,1764,1764,1764,1764,1764,1764,1764,1764,1764,1764,1764,1764,1764,1766,1766,1766,1766,1766,1766,1766,1766,1766,1766,1766,1766,1766,1766,1766,1768,1768,1768,1768,1768,1768,1768,1768,1768,1768,1768,1768,1768,1768,1768,1770,1770,1770,1770,1770,1770,1770,1770,1770,1770,1770,1770,1770,1770,1770,1772,1772,1772,1772,1772,1772,1772,1772,1772,1772,1772,1772,1772,1772,1772,1774,1774,1774,1774,1774,1774,1774,1774,1774,1774,1774,1774,1774,1774,1774,1776,1776,1776,1776,1776,1776,1776,1776,1776,1776,1776,1776,1776,1776,1776,1778,1778,1778,1778,1778,1778,1778,1778,1778,1778,1778,1778,1778,1778,1778,1780,1780,1780,1780,1780,1780,1780,1780,1780,1780,1780,1780,1780,1780,1780,1782,1782,1782,1782,1782,1782,1782,1782,1782,1782,1782,1782,1782,1782,1782,1784,1784,1784,1784,1784,1784,1784,1784,1784,1784,1784,1784,1784,1784,1784,1786,1786,1786,1786,1786,1786,1786,1786,1786,1786,1786,1786,1786,1786,1786,1788,1788,1788,1788,1788,1788,1788,1788,1788,1788.

Maybe somewhere else in the code, only the first n results are looped, ignoring duplicates.

Adding DISTINCT inside the GROUP_CONCAT clause seems to fix the problem. Not at all sure whether that is the proper way.

Steps to reproduce the issue

The script below creates a parent and child template, p and c, and PageTable fields c1_ref and c2_ref in the p template. Then it adds a parent page that contains 500 pages for both PageTable fields.

If run without GET params, it fetches the correct number of PageTable pages for both fields. If run with ?join, the first count is e.g. 350 instead of 500.

I haven't investigated this thoroughly, but it seems that the numbers might differ, maybe because the GROUP_CONCAT values are returned in semi-random order, depending on Mysql/Mariadb version and database state. To me it seems that having overally more rows, maybe in pages, makes the bug to reproduce more easily, even with a a few PageTable pages. If you are unable to reproduce, try increasing the number from 500 to e.g. 1000.

Setup/Environment

• ProcessWire version: latest dev
• (Optional) PHP version: 8.4
• Database version: at least Mysql 8.0.39 and 11.7.2-MariaDB-ubu2404

The script:

<?php

$processwirePath = '/var/www/html/';
include($processwirePath . 'index.php');
header('Content-Type: text/plain');

////// Install fixture templates, field and pages.
////// First, clean up previous run (if any).
$wire->database->exec("DELETE FROM pages WHERE templates_id IN (SELECT id FROM templates WHERE name IN ('p', 'c', 'c2'))");

// Remove templates
foreach(['p','c'] as $t) {
    $template = $templates->get($t);
    if ($template) $templates->delete($template);
}

// Remove fields
foreach(['c1_ref', 'c2_ref'] as $f) {
    $field = $fields->get($f);
    if ($field) $fields->delete($field);
}

// Create templates and field.
$pTemplate = $templates->add('p');
$pTemplate->save();

$cTemplate = $templates->add('c');
$cTemplate->save();

foreach(['c1_ref', 'c2_ref'] as $c) {
    $cField = $fields->makeItem();
    $cField->name = $c;
    $cField->type = 'PageTable';
    $cField->label = $c;
    $cField->template_id = $cTemplate->id;
    $cField->inputfield = "InputfieldPageTable";
    $cField->parent_id = 1;
    $cField->save();
    $pTemplate->fields->add($cField);
    $pTemplate->save();
}

// Create pages.
$parent = $pages->add('p', '/', 'parent-a');

for($i=1; $i <= 500; $i++) {
    $c1 = $pages->add('c', '/', "child1-$i");
    $parent->c1_ref->add($c1);

    $c2 = $pages->add('c', '/', "child2-$i");
    $parent->c2_ref->add($c2);

    $parent->save();
}

//////// Now everything is set up.

$loadOptions = isset($_GET['join']) ? ['joinFields' => ['c1_ref', 'c2_ref']] : [];

$result = $wire->pages->get($parent->id, ['loadOptions' => $loadOptions]);
echo "c1_ref count: " . count($result->c1_ref) . "\n";
echo "c2_ref count: " . count($result->c2_ref) . "\n";

[tuomassalo]

To me it seems that copying this function to FieldPagetable.php fixes this problem.

However, I don't think it's the optimal solution, since the same problem is also seen with $page->preload([$pagefield], ['useFieldtypeMulti => true]).

For preload, this fixes the problem:

diff --git wire/core/PagesLoader.php wire/core/PagesLoader.php
index 761c8d4b..9e60516f 100644
--- wire/core/PagesLoader.php
+++ wire/core/PagesLoader.php
@@ -2118,7 +2118,16 @@ class PagesLoader extends Wire {
                                if($options['useFieldtypeMulti'] && $fieldtype instanceof FieldtypeMulti) {
                                        $sep = FieldtypeMulti::multiValueSeparator;
                                        $orderBy = "ORDER BY $table.sort";
-                                       $selects[] = "GROUP_CONCAT($table.$colName $orderBy SEPARATOR '$sep') AS `{$table}__$colName`";
+
+                                       // NB: should probably use distinctAutoJoin condition here, like in
+                                       // FieldtypeMulti::getLoadQueryAutojoin(), and distinctAutoJoin should
+                                       // be set for all FieldtypePage/FieldetypePagetable fields.
+                                       $distinct = '';
+                                       if($fieldtype instanceof FieldtypePage) {
+                                               $distinct = "DISTINCT ";
+                                       }
+
+                                       $selects[] = "GROUP_CONCAT($distinct$table.$colName $orderBy SEPARATOR '$sep') AS `{$table}__$colName`";
                                } else {
                                        $selects[] = "$table.$colName AS {$table}__$colName";
                                }

If these fixes look good to you, maybe FieldtypePage and FieldtypePageTable should have a property that could be queried in both places - instead of distinctAutojoin that is only injected when needed by getLoadQueryAutojoin.

[tuomassalo]

I believe this issue is related to #928: adding DISTINCT reduces the likelihood of hitting group_concat_max_len limit. Anyway, I believe DISTINCT is worth adding.

We have been running the suggested fix for some months, without issues.

[matjazpotocnik]

@tuomassalo, can you verify the fix and close if it solves the problem?

[tuomassalo]

The patch fixes PageTable fields, but not a closely related porblem: Page fields and preload().

The test below adds 16 items to a Page field, but fails to retrieve them all. (I've modified group_concat_max_len to make the problem easier to aggravate the problem.)

<?php

include('/var/www/html/index.php');
header('Content-Type: text/plain');

////// Install fixture templates, field and pages.
////// First, clean up previous run (if any).
$wire->database->exec("DELETE FROM pages WHERE templates_id IN (SELECT id FROM templates WHERE name IN ('p', 'c', 'c2'))");

// Remove templates
foreach(['p','c'] as $t) {
    $template = $templates->get($t);
    if ($template) $templates->delete($template);
}

// Remove fields
foreach(['c1_ref', 'c2_ref'] as $f) {
    $field = $fields->get($f);
    if ($field) $fields->delete($field);
}

// Create templates and field.
$pTemplate = $templates->add('p');
$pTemplate->save();

$cTemplate = $templates->add('c');
$cTemplate->save();

foreach(['c1_ref', 'c2_ref'] as $c) {
    $cField = $fields->makeItem();
    $cField->name = $c;
    $cField->type = 'Page';
    $cField->save();
    $pTemplate->fields->add($cField);
    $pTemplate->save();
}

// If ids have 4 digits, 2 * 16 * (4+2) = 192 bytes should be enough
// for the null+comma delimited list of ids returned by GROUP_CONCAT.
$wire->database->exec("SET SESSION group_concat_max_len=256");

// Create pages.
$parent = $pages->add('p', '/', 'parent-a');

for($i=1; $i <= 16; $i++) {
    $c1 = $pages->add('c', '/', "child1-$i");
    $c2 = $pages->add('c', '/', "child2-$i");

    $parent->c1_ref->add($c1);
    $parent->c2_ref->add($c2);
}
$parent->save();

$parent_id = $parent->id;

$wire->pages->uncacheAll();

$result = $wire->pages->get($parent_id);

$result->preload(['c1_ref', 'c2_ref'], ['useFieldtypeMulti' => true]);

echo "c1_ref count: " . count($result->c1_ref) . "\n";
echo "c2_ref count: " . count($result->c2_ref) . "\n";