Skip to content

feat(contexts): keep widgets in step with pages and applications #3132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rubenvdlinde wants to merge 1 commit into from
Open

feat(contexts): keep widgets in step with pages and applications #3132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 50 additions & 5 deletions lib/Db/ContextMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ protected function getFindContextBaseQuery(?string $userId): IQueryBuilder {
$qb->select(
'c.*',
'r.id as node_rel_id', 'r.node_id', 'r.node_type', 'r.permissions',
'p.id as page_id', 'p.page_type',
'p.id as page_id', 'p.page_type', 'p.technical_name as page_technical_name', 'p.title as page_title', 'p.icon as page_icon', 'p.description as page_description',

@enjeck enjeck Oct 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we join only the start page here, and load the page fields in a separate query? Otherwise, imagine someone opens just a start page, we might be wasting time processing stuff that doesn't matter for them? Maybe i misunderstand

@rubenvdlinde rubenvdlinde Nov 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You understood it right: every grid page multiplied the joined rows by the number of tables and shares. The base query now joins only the start page, as before this PR. findAll() and findById() load the other pages in one extra query for all applications in the result. findForNavBar() and findAllContainingNode() skip it, because the navigation shows names and the permission checks only read the nodes.

'pc.id as content_id', 'pc.order',
'n.display_mode as display_mode_default',
's.id as share_id', 's.receiver', 's.receiver_type'
Expand All @@ -58,7 +58,11 @@ protected function getFindContextBaseQuery(?string $userId): IQueryBuilder {
));
}

$qb->leftJoin('c', 'tables_contexts_page', 'p', $qb->expr()->eq('c.id', 'p.context_id'));
// only the start page: other pages are loaded by withOtherPages()
$qb->leftJoin('c', 'tables_contexts_page', 'p', $qb->expr()->andX(
$qb->expr()->eq('c.id', 'p.context_id'),
$qb->expr()->eq('p.page_type', $qb->createNamedParameter(Page::TYPE_STARTPAGE)),
));
$qb->leftJoin('p', 'tables_contexts_page_content', 'pc', $qb->expr()->eq('p.id', 'pc.page_id'));
$qb->leftJoin('c', 'tables_contexts_rel_context_node', 'r', $qb->expr()->eq('c.id', 'r.context_id'));
$qb->leftJoin('s', 'tables_contexts_navigation', 'n', $qb->expr()->andX(
Expand All @@ -72,6 +76,7 @@ protected function getFindContextBaseQuery(?string $userId): IQueryBuilder {
));

$qb->orderBy('pc.order', 'ASC');
$qb->addOrderBy('r.id', 'ASC');

return $qb;
}
Expand Down Expand Up @@ -131,7 +136,11 @@ protected function formatResultRows(array $rows, ?string $userId) {
$carry[$item['page_id']] ??= ['content' => []];
$carry[$item['page_id']]['id'] = (int)$item['page_id'];
$carry[$item['page_id']]['page_type'] = $item['page_type'];
if ($item['node_rel_id'] !== null) {
$carry[$item['page_id']]['technical_name'] = $item['page_technical_name'];
$carry[$item['page_id']]['title'] = $item['page_title'];
$carry[$item['page_id']]['icon'] = $item['page_icon'];
$carry[$item['page_id']]['description'] = $item['page_description'];
if ($item['node_rel_id'] !== null && $item['content_id'] !== null) {
$carry[$item['page_id']]['content'][$item['content_id']] = [
'order' => (int)$item['order'],
'node_rel_id' => (int)$item['node_rel_id']
Expand All @@ -152,7 +161,7 @@ public function findAll(?string $userId = null): array {
$qb = $this->getFindContextBaseQuery($userId);

$result = $qb->executeQuery();
$r = $result->fetchAllAssociative();
$r = $this->withOtherPages($result->fetchAllAssociative());

$contextIds = [];
foreach ($r as $row) {
Expand Down Expand Up @@ -234,7 +243,7 @@ public function findById(int $contextId, ?string $userId = null): Context {
$qb->andWhere($qb->expr()->eq('c.id', $qb->createNamedParameter($contextId, IQueryBuilder::PARAM_INT)));

$result = $qb->executeQuery();
$r = $result->fetchAllAssociative();
$r = $this->withOtherPages($result->fetchAllAssociative());

@AndyScherzinger AndyScherzinger Nov 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on #3132 (comment): the reply and the PR description say the permission checks skip the extra page query, but the context checks go through findById(): PermissionsService::canAccessContextById() and canManageContextById() (used by PermissionMiddleware for RequirePermission with typeParam: 'context' and for @CanManageContext) and assertOwnerPermission(). TableService::findAll() and ViewService::findSharedViewsWithMe(), which build the table and view lists on every app load, go through findAll() and only read getNodes(); so do addNodeToContextById(), addNodeRelToPage() (start page only) and compareSchemeChanges().

On the dev instance the profiler shows SELECT ... FROM oc_tables_contexts_page WHERE context_id IN (...) AND page_type <> 'startpage' on GET /apps/tables/table and GET /apps/tables/view, and four times on GET /api/2/contexts/{id}/scheme/export (three permission checks plus getScheme()).

Each query is small, but they sit on hot paths, while only the context responses, getScheme(), the migrator and PageService (#3133) read the other pages. Loading them explicitly there, through a withAllPages flag on findById()/findAll() or a PageMapper::findByContextIds() that returns finished page arrays instead of synthetic join rows, keeps the other callers at one query. It would also make visible which entities are complete: today findForNavBar() and findAllContainingNode() return contexts whose pages hold only the start page, although the comment on line 61 reads as if every finder added the rest.


if (empty($r)) {
throw new NotFoundError('Context does not exist');
Expand Down Expand Up @@ -304,6 +313,42 @@ public function findIdToNameMap(array $ids): array {
return $map;
}

/**
* Adds a row for every page other than the start page of the contexts in the rows,
* holding only the context id and the page fields.
*
* @throws Exception
*/
private function withOtherPages(array $rows): array {
$contextIds = array_values(array_unique(array_map(static fn (array $row): int => (int)$row['id'], $rows)));
if ($contextIds === []) {
return $rows;
}
$qb = $this->db->getQueryBuilder();
$qb->select('id', 'context_id', 'page_type', 'technical_name', 'title', 'icon', 'description')
->from('tables_contexts_page')
->where($qb->expr()->in('context_id', $qb->createNamedParameter($contextIds, IQueryBuilder::PARAM_INT_ARRAY)))
->andWhere($qb->expr()->neq('page_type', $qb->createNamedParameter(Page::TYPE_STARTPAGE)))
->orderBy('id');
$result = $qb->executeQuery();
while ($page = $result->fetchAssociative()) {
$rows[] = [
'id' => $page['context_id'],
'share_id' => null,
'node_rel_id' => null,
'content_id' => null,
'page_id' => $page['id'],
'page_type' => $page['page_type'],
'page_technical_name' => $page['technical_name'],
'page_title' => $page['title'],
'page_icon' => $page['icon'],
'page_description' => $page['description'],
];
}
$result->closeCursor();
Comment on lines +330 to +348

@AndyScherzinger AndyScherzinger Nov 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same Oracle limit as in #3131 (comment), but this list covers whole instances: findAll(null) (occ tables:contexts:list without a user, and occ tables:list through TableService::findAll('')) puts the id of every application into this one IN (), and so does every app load of a user who owns or receives more than 1000 applications. AGENTS.md asks for chunks of DB_CHUNK_SIZE, which this class already defines and uses in findIdToNameMap().

Reproduced on the dev instance with 1001 throwaway applications: occ tables:contexts:list <owner> logs "More than 1000 expressions in a list are not allowed on Oracle." at error level from withOtherPages().

Suggested change
->where($qb->expr()->in('context_id', $qb->createNamedParameter($contextIds, IQueryBuilder::PARAM_INT_ARRAY)))
->andWhere($qb->expr()->neq('page_type', $qb->createNamedParameter(Page::TYPE_STARTPAGE)))
->orderBy('id');
$result = $qb->executeQuery();
while ($page = $result->fetchAssociative()) {
$rows[] = [
'id' => $page['context_id'],
'share_id' => null,
'node_rel_id' => null,
'content_id' => null,
'page_id' => $page['id'],
'page_type' => $page['page_type'],
'page_technical_name' => $page['technical_name'],
'page_title' => $page['title'],
'page_icon' => $page['icon'],
'page_description' => $page['description'],
];
}
$result->closeCursor();
->where($qb->expr()->in('context_id', $qb->createParameter('contextIds')))
->andWhere($qb->expr()->neq('page_type', $qb->createNamedParameter(Page::TYPE_STARTPAGE)))
->orderBy('id');
foreach (array_chunk($contextIds, self::DB_CHUNK_SIZE) as $chunk) {
$qb->setParameter('contextIds', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
$result = $qb->executeQuery();
while ($page = $result->fetchAssociative()) {
$rows[] = [
'id' => $page['context_id'],
'share_id' => null,
'node_rel_id' => null,
'content_id' => null,
'page_id' => $page['id'],
'page_type' => $page['page_type'],
'page_technical_name' => $page['technical_name'],
'page_title' => $page['title'],
'page_icon' => $page['icon'],
'page_description' => $page['description'],
];
}
$result->closeCursor();
}

return $rows;
}

protected function applyOwnedOrSharedQuery(IQueryBuilder $qb, string $userId): void {
$sharedToConditions = $qb->expr()->orX();

Expand Down
2 changes: 2 additions & 0 deletions lib/Db/Page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ class Page extends Entity implements \JsonSerializable {
public const TYPE_GRID = 'grid';
public const START_PAGE_NAME = 'dashboard';
public const DEFAULT_NAME_PREFIX = 'page_';
public const TITLE_MAX_LENGTH = 200;
public const DESCRIPTION_MAX_LENGTH = 2000;

protected ?int $contextId = null;
protected ?string $pageType = null;
Expand Down
27 changes: 27 additions & 0 deletions lib/Db/WidgetSourceMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,33 @@ public function findNodeRelIdsByWidgetIds(array $widgetIds): array {
return $sources;
}

/**
* The given node relations that belong to the application of a page.
*
* @param int[] $nodeRelIds
* @return int[]
* @throws Exception
*/
public function filterNodeRelIdsOfPageContext(int $pageId, array $nodeRelIds): array {
if ($nodeRelIds === []) {
return [];
}
$qb = $this->db->getQueryBuilder();
$qb->select('r.id')
->from('tables_contexts_rel_context_node', 'r')
->innerJoin('r', 'tables_contexts_page', 'p', $qb->expr()->eq('p.context_id', 'r.context_id'))
->where($qb->expr()->eq('p.id', $qb->createNamedParameter($pageId, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->in('r.id', $qb->createNamedParameter($nodeRelIds, IQueryBuilder::PARAM_INT_ARRAY)));

$result = $qb->executeQuery();
$ids = [];
while ($row = $result->fetchAssociative()) {
$ids[] = (int)$row['id'];
}
$result->closeCursor();
return $ids;
}

/**
* Points a widget at a node relation, or removes its source when null.
*
Expand Down
136 changes: 127 additions & 9 deletions lib/Service/ContextService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use OCA\Tables\Errors\InternalError;
use OCA\Tables\Errors\NotFoundError;
use OCA\Tables\Errors\PermissionError;
use OCA\Tables\Helper\IconHelper;
use OCA\Tables\Model\ColumnSettings;
use OCA\Tables\Model\ContextScheme;
use OCA\Tables\Model\FilterSet;
Expand Down Expand Up @@ -63,6 +64,8 @@ public function __construct(
protected IURLGenerator $urlGenerator,
private TableMapper $tableMapper,
private ViewMapper $viewMapper,
private WidgetService $widgetService,
private GridWidgetTypes $gridWidgetTypes,
) {
}

Expand Down Expand Up @@ -342,6 +345,7 @@ public function delete(int $contextId, string $userId): Context {
$this->shareService->deleteAllForContext($context);
$this->contextNodeRelMapper->deleteAllByContextId($context->getId());
$pageIds = $this->pageMapper->getPageIdsForContext($context->getId());
$this->widgetService->deleteByPageIds($pageIds);
foreach ($pageIds as $pageId) {
$this->pageContentMapper->deleteByPageId($pageId);
$this->pageMapper->deleteByPageId($pageId);
Expand All @@ -359,6 +363,7 @@ public function deleteNodeRel(int $nodeId, int $nodeType): void {
try {
$nodeRelIds = $this->contextNodeRelMapper->getRelIdsForNode($nodeId, $nodeType);
$this->atomic(function () use ($nodeRelIds): void {
$this->widgetService->deleteSourcesByNodeRelIds($nodeRelIds);
$this->pageContentMapper->deleteByNodeRelIds($nodeRelIds);
$this->contextNodeRelMapper->deleteByNodeRelIds($nodeRelIds);
}, $this->dbc);
Expand Down Expand Up @@ -454,9 +459,12 @@ public function addNodeToContextAndStartpage(int $contextId, int $nodeId, int $n
* @throws Exception
*/
public function removeNodeFromContextAndPages(int $nodeRelationId): array {
$nodeRelation = $this->removeNodeFromContextById($nodeRelationId);
$contents = $this->removeNodeRelFromAllPages($nodeRelation);
return [$nodeRelation, $contents];
return $this->atomic(function () use ($nodeRelationId): array {
$this->widgetService->deleteSourcesByNodeRelIds([$nodeRelationId]);

@AndyScherzinger AndyScherzinger Nov 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update() calls this for every removed node (line 240) before it checks canManageNodeById() for the added ones (line 251), and each call commits on its own. A request refused by that check therefore still removes the nodes, and with this PR it also deletes the source of every data widget that showed them. Re-adding the table creates a new node relation, so those widgets stay without a source: the partial update itself is older, but now it also loses widget configuration.

Reproduced on the dev instance: an application with one table on its start page and a data widget showing it, then PUT /api/2/contexts/{id} with {"name": "renamed", "nodes": [{"id": 999999, "type": 0, "permissions": 1}]}. The response is 403, but the node relation and the start page content are gone and the widget's widget_source row is deleted; only the name is unchanged.

Checking the added nodes before the removal loop, or running all of update() in one atomic() (nested transactions are savepoints in Nextcloud), keeps a refused request from changing anything.

$nodeRelation = $this->removeNodeFromContextById($nodeRelationId);
$contents = $this->removeNodeRelFromAllPages($nodeRelation);

@AndyScherzinger AndyScherzinger Nov 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new transaction takes its locks in the opposite order to deleteNodeRel(): here the relation is deleted before its page contents, there the contents before the relations. Removing a table or view from an application while that table or view is being deleted can now deadlock (whenever no widget shows it, the first DELETE locks nothing); before, each statement here committed on its own and never held a lock while waiting for another. If deleteNodeRel() is the victim, its catch only logs, TableService::delete() deletes the table anyway, and its node relations, page contents and widget sources stay behind in the other applications. TableService::findAll() then throws a NotFoundError for that node, so the table list answers 404 for everyone who can see one of those applications (checked on the dev instance by leaving such a relation behind: GET /apps/tables/table returns 404 until the application is deleted).

removeNodeRelFromAllPages() also still catches and only logs each failed page-content DELETE. That suited autocommit; inside the transaction it hides the error from atomic(), which commits anyway (on PostgreSQL an aborted transaction then ends in a rollback that PDO doesn't report).

Deleting the contents with pageContentMapper->deleteByNodeRelIds([$nodeRelationId]) before the relation, in deleteNodeRel()'s order, and dropping that catch closes both; findByNodeRelation() can still provide the return value.

return [$nodeRelation, $contents];
}, $this->dbc);
}

/**
Expand Down Expand Up @@ -666,15 +674,26 @@ public function importContext(Table $table, array $context, int $tableOldId): ?C

// insert nodes
$nodeRelMap = [];
// the relations of the table being imported, the only node a widget source can keep
$sourceRelMap = [];
Comment on lines +677 to +678

@enjeck enjeck Oct 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have a bug: If I have an application with 2 tables and export an account. When i import, the application is created twice 🤔

@rubenvdlinde rubenvdlinde Nov 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this. It's older than this PR: the migrator called importContext() once per imported table and pointed every node at that one table. #3200 imports the tables and views first and then each application once, with every node on its own table or view.

if (!empty($context['nodes'])) {
foreach ($context['nodes'] as $nodeArr) {
$nodeType = $nodeArr['node_type'] ?? $nodeArr['nodeType'] ?? 0;
$contextNodeRel = new ContextNodeRelation();
$contextNodeRel->setContextId($newContext->getId());
$contextNodeRel->setNodeId($table->getId());
$contextNodeRel->setNodeType($nodeArr['node_type'] ?? $nodeArr['nodeType'] ?? 0);
$contextNodeRel->setNodeType($nodeType);
$contextNodeRel->setPermissions($nodeArr['permissions'] ?? 0);
$contextNodeRel = $this->contextNodeRelMapper->insert($contextNodeRel);
$nodeRelMap[$nodeArr['id'] ?? null] = $contextNodeRel;
$oldRelId = self::importedKey($nodeArr['id'] ?? null);
if ($oldRelId === null) {
continue;
}
$nodeRelMap[$oldRelId] = $contextNodeRel;
$oldNodeId = $nodeArr['node_id'] ?? $nodeArr['nodeId'] ?? null;
if (is_numeric($nodeType) && (int)$nodeType === Application::NODE_TYPE_TABLE && is_numeric($oldNodeId) && (int)$oldNodeId === $tableOldId) {
$sourceRelMap[$oldRelId] = $contextNodeRel;
}
}
}

Expand All @@ -687,6 +706,9 @@ public function importContext(Table $table, array $context, int $tableOldId): ?C
$page = new Page();
$page->setContextId($newContext->getId());
$page->setPageType($pageArr['page_type'] ?? $pageArr['pageType'] ?? '');
$page->setTitle($this->importedText($pageArr['title'] ?? null, Page::TITLE_MAX_LENGTH, 'page title'));
$page->setIcon($this->importedIcon($pageArr['icon'] ?? null));
$page->setDescription($this->importedText($pageArr['description'] ?? null, Page::DESCRIPTION_MAX_LENGTH, 'page description'));
if ($technicalNames[$pageKey] === null) {
$page = $this->pageMapper->insertWithDefaultName($page);
} else {
Expand All @@ -698,15 +720,26 @@ public function importContext(Table $table, array $context, int $tableOldId): ?C
// insert page conten
if (!empty($pageArr['content'])) {
foreach ($pageArr['content'] as $contentArr) {
$oldNodeRelId = self::importedKey($contentArr['node_rel_id'] ?? $contentArr['nodeRelId'] ?? null);

@AndyScherzinger AndyScherzinger Nov 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

importedKey() now guards the node and content ids, but the page id a few lines up still goes into an array key unchecked: $pageMap[$pageArr['id'] ?? null] = $page; (line 718). A page whose id is an object or an array in contexts.json throws TypeError: Cannot access offset of type array on array, and TablesMigrator::import() rolls back the whole account import, which is the case this PR otherwise turns into skips.

Reproduced on the dev instance (PHP 8.5): importContext() with a grid page whose id is {"x": 1} aborts with that TypeError.

$pageMap is written but never read, also further up the stack, so removing lines 701 and 718 fixes it. If it stays for referencing pages by their exported id, as suggested in #3131 (comment), self::importedKey() would guard it like the other two maps.

$newNodeRel = $oldNodeRelId !== null ? ($nodeRelMap[$oldNodeRelId] ?? null) : null;
if ($newNodeRel === null) {
continue;
}
$pageContent = new PageContent();
$pageContent->setPageId($page->getId());
$oldNodeRelId = $contentArr['node_rel_id'] ?? $contentArr['nodeRelId'] ?? null;
$newNodeRel = $nodeRelMap[$oldNodeRelId] ?? null;
$pageContent->setNodeRelId($newNodeRel ? $newNodeRel->getId() : null);
$pageContent->setNodeRelId($newNodeRel->getId());
$pageContent->setOrder($contentArr['order'] ?? 100);
$this->pageContentMapper->insert($pageContent);
}
}

$widgets = is_array($pageArr['widgets'] ?? null) ? array_values($pageArr['widgets']) : [];

@AndyScherzinger AndyScherzinger Nov 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Widgets are imported for every importable page, the start page included, while PageService::update() in #3133 refuses widgets on anything but a grid page ("Only grid pages hold widgets.") and delete() refuses the start page. An export whose start page lists widgets (edited, or written by a later version) therefore leaves widgets on the start page that no API call can change or remove, and every later export writes them out again.

Reproduced on the dev instance by calling importContext() with a start page that holds a header and a data widget: both were stored on the start page, the data widget with its remapped source.

importWidget()'s docblock promises to skip what the API wouldn't save, so checking the page type here keeps that promise. (The mirror case, content on a grid page, is imported too, but withOtherPages() never reads it back.)

Suggested change
$widgets = is_array($pageArr['widgets'] ?? null) ? array_values($pageArr['widgets']) : [];
$widgets = $page->getPageType() === Page::TYPE_GRID && is_array($pageArr['widgets'] ?? null) ? array_values($pageArr['widgets']) : [];

if (count($widgets) > GridWidgetTypes::MAX_WIDGETS_PER_PAGE) {
$this->logger->warning('Skipping ' . (count($widgets) - GridWidgetTypes::MAX_WIDGETS_PER_PAGE) . ' imported widgets: a page holds at most ' . GridWidgetTypes::MAX_WIDGETS_PER_PAGE . '.');
}
foreach (array_slice($widgets, 0, GridWidgetTypes::MAX_WIDGETS_PER_PAGE) as $index => $widgetArr) {
$this->importWidget($page->getId(), $index, $widgetArr, $sourceRelMap);
}
}
}

Expand Down Expand Up @@ -743,6 +776,91 @@ private function importedPageNames(array $pages): array {
return $names;
}

/**
* The pages of an application with the widgets of each page under `widgets`.
*
* @throws Exception
*/
public function getPagesWithWidgets(Context $context): array {
$pages = $context->getPages() ?? [];
$widgets = $this->widgetService->findByPageIds(array_keys($pages));
foreach ($pages as $pageId => $page) {
$pages[$pageId]['widgets'] = $widgets[$pageId] ?? [];
}
return $pages;
}

/**
* Inserts an exported widget on a page. Its source is remapped to the imported node
* relation of the table being imported, and dropped when it showed another node. A
* widget that would not be saved through the API, for its type, settings, title,
* description, place on the grid or the shape of its source, is skipped.
*
* @param array<array-key, ContextNodeRelation> $nodeRelMap imported node relations of the table by their exported id
* @throws Exception
* @throws \JsonException
*/
private function importWidget(int $pageId, int $index, mixed $widgetArr, array $nodeRelMap): void {
if (!is_array($widgetArr) || !is_string($widgetArr['type'] ?? null)) {
return;
}
try {
$hasSource = $this->gridWidgetTypes->get($widgetArr['type'])['hasSource'];

@AndyScherzinger AndyScherzinger Nov 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The per-widget rules are assembled by each caller: here five GridWidgetTypes calls plus a source check, in #3133 PageService::readWidgets() the same five plus its own rules, while WidgetService::insert()/syncPage() store whatever arrays they receive (eight positional parameters in insert()). The paths already disagree on rules that should be shared: widgets on non-grid pages (see line 736), and later in the stack the tabs limit (871f122) exists only in readWidgets(), so an imported tabs widget with more than 20 tabs is stored and every later save of its page fails until the tabs are cut down.

AGENTS.md asks for structured input to be parsed into a value object at the boundary and for service methods to take value objects instead of raw arrays. A WidgetInput::createFromInputArray(), or one GridWidgetTypes::readWidget(), used by both callers, with the import catching its exception to skip the widget, keeps the rules in one place and lets WidgetService rely on what it gets. The class docblock there ("Callers ... run writes in a transaction") also no longer matches syncPage(), which now opens its own.

Smaller, in the skip handling here: a widget that isn't an object or has no type is dropped without a log line (line 804), while every other skip is logged with ['exception' => $e], i.e. a full stack trace at warning level per skipped widget, up to 100 per page, for a case the code expects. The source check also throws a BadRequestError only to catch it two lines later; a plain if that logs and returns reads more directly.

$settings = $this->gridWidgetTypes->sanitizeSettings($widgetArr['type'], $widgetArr['settings'] ?? null);
$title = $this->gridWidgetTypes->validateTitle($index, $widgetArr['title'] ?? null);
$description = $this->gridWidgetTypes->validateDescription($index, $widgetArr['description'] ?? null);
$position = $this->gridWidgetTypes->validatePosition($index, $widgetArr);
$source = $widgetArr['source'] ?? null;
$oldNodeRelId = is_array($source) ? ($source['nodeRelId'] ?? null) : null;
if (($source !== null && !is_array($source)) || ($oldNodeRelId !== null && !is_int($oldNodeRelId))) {
throw new BadRequestError('The source of widget ' . $index . ' must be an object with a numeric nodeRelId.');
}
} catch (BadRequestError $e) {
$this->logger->warning('Skipping an imported widget: ' . $e->getMessage(), ['exception' => $e]);
return;
}
$newNodeRel = $hasSource && $oldNodeRelId !== null ? ($nodeRelMap[$oldNodeRelId] ?? null) : null;
$this->widgetService->insert(
$pageId,
$widgetArr['type'],
$title,
$position,
$settings,
$newNodeRel?->getId(),
$this->importedIcon($widgetArr['icon'] ?? null),
$description,
);
}

/**
* An exported id usable as an array key, or null for a value of another type.
*/
private static function importedKey(mixed $id): int|string|null {
return is_int($id) || is_string($id) ? $id : null;
}

/**
* An exported text, trimmed, or null when it is empty, not a text or too long.
*/
private function importedText(mixed $text, int $maxLength, string $what): ?string {
if ($text === null) {
return null;
}
if (!is_string($text) || mb_strlen(trim($text)) > $maxLength) {
$this->logger->warning('Dropping an imported ' . $what . ': it is not a text of at most ' . $maxLength . ' characters.');
return null;
}
$text = trim($text);
return $text === '' ? null : $text;
}

/**
* An exported icon name, or null when this instance has no such icon.
*/
private function importedIcon(mixed $icon): ?string {
return is_string($icon) && IconHelper::isValid($icon) ? $icon : null;
}

/**
* @param int $contextId
*
Expand Down Expand Up @@ -792,7 +910,7 @@ public function getScheme(int $contextId, string $userId): ContextScheme {
$context->getIcon(),
$context->getDescription(),
array_values($nodes),
$context->getPages(),
$this->getPagesWithWidgets($context),

@enjeck enjeck Oct 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scheme export now includes pages and widgets, but the scheme import ignores them though. Why?

@rubenvdlinde rubenvdlinde Nov 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a gap: I added the export side and left the import for later. #3201 imports the pages and widgets of a scheme, with a Pages switch on the import screen next to the Menu one.

array_values($tables),
);
}
Expand Down
Loading
Loading