Skip to content

Order the CMS page links by the pages' own position - #209

Draft
boo-code wants to merge 1 commit into
PrestaShop:devfrom
boo-code:fix/order-cms-links-by-page-position-16134
Draft

boo-code wants to merge 1 commit into
PrestaShop:devfrom
boo-code:fix/order-cms-links-by-page-position-16134

Conversation

@boo-code

Copy link
Copy Markdown
Questions Answers
Description? A link block stores its selected CMS page ids as a JSON array in ps_link_block.content, in whatever order the configuration form submitted them, and LinkBlockPresenter::makeCmsLinks() renders that array order. The page position from Design > Pages is never consulted, so reordering pages there leaves the footer as it was - while the block's own configuration screen lists them in the new order, which is what makes the current behaviour look like a display bug rather than a stored preference. The links are now ordered by the pages' own (id_cms_category, position), which is the order the back office shows, with the id as a tie-break because position is only unique within a category.
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#16134.
How to test? With all CMS pages checked in a footer link block, note the footer order. Reorder the pages in Design > Pages, clear the shop cache, reload the front office: the footer follows the new order. On dev it keeps the old one. The uncheck-all / save / recheck-all / save workaround from the issue is no longer needed.

What the change is, and what it deliberately is not

makeCmsLinks() kept its loop unchanged - the same new \CMS((int) $cmsId) construction and the
same null !== $cms->id && $cms->active filter - and only collects the pages before presenting
them, so that they can be sorted first. That is deliberate: which links appear must not change,
only their sequence, and keeping the construction identical is what makes that provable rather
than argued. position is already loaded by the constructor, so the sort costs no query.

Sorting on the page position is the direction @mark-app proposed on PrestaShop/PrestaShop#29197
in 2022. Two differences: the CMS objects are sorted before presentation instead of a position
key being added to the presented link, which is the template's contract, and the order includes
the category, for the reason below.

It is not the drag-and-drop ordering proposed on the issue in 2022. That would give the block an
order of its own, editable in the block's form; this makes the page position the single source.
Both are defensible, and the reason to take this one now is that it is the smaller change and it
removes an inconsistency that exists regardless of which design wins: the block's configuration
screen already lists the pages in Design > Pages order, so the front office contradicting it is
wrong under either model. If the drag-and-drop ordering is specified and built, it supersedes this
sort, and the migration question it has to answer - what order existing blocks start from - is
easier to answer once the answer is "the page order" rather than "whatever was submitted".

One consequence to state plainly: a shop whose footer order was arranged through the
uncheck-all / recheck-all workaround will reorder on upgrade, because that arrangement is exactly
the stored array order this stops honouring. There is no way to keep both - the arrangement and
the page order are the two orders that disagree.

Ordering across CMS categories

ps_cms.position is scoped to id_cms_category, so position alone would interleave pages from
different categories by numbers that mean nothing relative to each other. Design > Pages shows one
category at a time, so there is no back-office order to copy across categories; grouping by
category and then position is the deterministic reading of "the order the back office shows",
and it keeps each category's pages contiguous. Measured with a page moved into a second category:
the four pages of category 1 render in position order, then the page from the other category.

Verification

No PHPUnit harness in this module (tests/ holds phpstan only, composer.json has no scripts), so
the behaviour was measured on a running 9.2.0 shop with hummingbird, one language, all five demo
CMS pages checked in the footer block, comparing the base file against the branch file in place.

Positions set so Design > Pages reads Secure payment, Delivery, Legal Notice, Terms and conditions of use, About us (page ids 5, 1, 2, 3, 4) while the block still stored 1,2,3,4,5:

footer order
base 1, 2, 3, 4, 5
branch 5, 1, 2, 3, 4
base again 1, 2, 3, 4, 5

The third row is the mutation control - restoring the file restores the old order, so the branch is
what changed it and not the cache clear that each measurement needs.

Set identity, which is the control that matters for a change that reshuffles a list: with one page
deactivated, base and branch both render four links, and comparing the full anchor tuple - page id,
block id, href and visible text - the two sets are identical, only the sequence differs. Same
result in the cross-category case. The extraction asserts it found something before comparing, so
an empty-vs-empty pass cannot be mistaken for agreement.

tests/phpstan/phpstan.neon against the local 9.2.0 checkout ("Detected PS version 9.2.0"): one
error, getCategoryMetas() expects string, null given, at the same call on both base and branch -
pre-existing, unrelated, and unchanged by this. Zero new errors. php-cs-fixer with the module's
own config reports no diff; verified it reads the file rather than passing vacuously by injecting
$this->sortByPagePosition( $pages ); with stray spaces and seeing it reported.

A second, separate defect found while measuring this

The widget's template cache is never invalidated by a page change, so even with this fix the
corrected order appears only after the shop cache is cleared. Measured: with PS_SMARTY_CACHE on,
editing ps_link_block.content directly left the footer unchanged until var/cache was cleared.
The module's only invalidation hook is actionGeneralPageSave, and that hook is declared in core's
install-dev/data/xml/hook.xml but dispatched nowhere in 9.2.x. CMS page positions are updated
through PrestaShopAdminController::updateGridPosition() into GridPositionUpdater::update(),
which is raw SQL with no hook and no cache clear, so actionObjectCmsUpdateAfter does not fire
either.

That is a core-side gap plus a module hook with no producer, not something this PR can close, and
it applies to every kind of change to a page rather than to ordering. Worth its own issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Changing order of Pages is not reflesting in the footer

1 participant