Repository navigation
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ps_link_block.content, in whatever order the configuration form submitted them, andLinkBlockPresenter::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 becausepositionis only unique within a category.devit 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 samenew \CMS((int) $cmsId)construction and thesame
null !== $cms->id && $cms->activefilter - and only collects the pages before presentingthem, 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.
positionis 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
CMSobjects are sorted before presentation instead of apositionkey 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.positionis scoped toid_cms_category, so position alone would interleave pages fromdifferent 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.jsonhas no scripts), sothe 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 stored1,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,
hrefand visible text - the two sets are identical, only the sequence differs. Sameresult 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.neonagainst the local 9.2.0 checkout ("Detected PS version 9.2.0"): oneerror,
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-fixerwith the module'sown 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_CACHEon,editing
ps_link_block.contentdirectly left the footer unchanged untilvar/cachewas cleared.The module's only invalidation hook is
actionGeneralPageSave, and that hook is declared in core'sinstall-dev/data/xml/hook.xmlbut dispatched nowhere in 9.2.x. CMS page positions are updatedthrough
PrestaShopAdminController::updateGridPosition()intoGridPositionUpdater::update(),which is raw SQL with no hook and no cache clear, so
actionObjectCmsUpdateAfterdoes not fireeither.
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.