Skip to content

Create the consent rows when the module is installed, not when its page is opened - #248

Draft
boo-code wants to merge 1 commit into
PrestaShop:devfrom
boo-code:fix/create-consent-rows-on-hook-registration-31789
Draft

Create the consent rows when the module is installed, not when its page is opened#248
boo-code wants to merge 1 commit into
PrestaShop:devfrom
boo-code:fix/create-consent-rows-on-hook-registration-31789

Conversation

@boo-code

@boo-code boo-code commented Sep 5, 2026

Copy link
Copy Markdown
Questions Answers
Description? ps_psgdpr_consent holds one row per module that wants a consent checkbox, and hookDisplayGDPRConsent() renders nothing unless GDPRConsent::getConsentActive($id_module) finds one. The only code that creates those rows is getRegisteredModules(), whose only caller is getContent() - so the checkbox appears on the front office as a side effect of an employee opening the module's configuration page, and a shop where nobody opened it never shows one. Measured on 9.2 with the bundled version: /contact-us carries no consent markup with the table empty and the checkbox once the rows exist. The registration now runs on install and on upgrade as well.
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#31789
How to test? On a shop where ps_psgdpr_consent is empty and the module configuration page has never been opened, look at the contact form: before this change there is no consent checkbox, after installing or upgrading with this change there is. Opening the configuration page still registers any module added later.

Root cause (measured, not read)

ps_psgdpr_consent holds one row per module that wants a consent checkbox. hookDisplayGDPRConsent()
returns '' unless GDPRConsent::getConsentActive($id_module) finds one. The only code that creates those
rows is getRegisteredModules(), and it is called from getContent() and nowhere else - i.e. as a side
effect of an employee opening the module configuration page.

A/B on the 9.2 shop with the bundled psgdpr 1.4.3, nothing else changed between the two lines:

ps_psgdpr_consent EMPTY   ->  /contact-us  58,177 bytes, 0 gdpr markers
  ran getRegisteredModules() once, the one thing getContent() does
ps_psgdpr_consent 4 rows  ->  /contact-us  61,155 bytes, psgdpr_consent_checkbox present

dev (2.0.3) has the identical shape: getRegisteredModules() still has getContent() as its only caller.

Why the thread could never agree

A state precondition, not a code path - it reproduces only on shops where nobody opened that page, which is
why AureRita and MatShir saw checkboxes on the same versions where RosaBenouamer, paulnoelcholot and
djoelleuch did not.

Honest scope limit: the reporter's steps say they ticked all five toggles, i.e. they did open the page,
so this may not be their path. I cleared the save path as the alternative explanation: the per-module
switches are paired radios (dataConsent.tpl:169/172, value="1"/value="0"), so both states POST and
submitDataConsent() cannot write active=0 by omission. Said so in the upstream comment rather than
letting the tidier story absorb the discrepancy.

zodit/Miguel86/carlos-blazquez describe a different cause with the same symptom (psgdpr missing from the
displayGDPRConsent hook); upgrade/upgrade-1.4.3.php already repairs that one.

The fix

  1. actionModuleRegisterHookAfter listener - creates the row when a module declares itself on
    registerGDPRConsent, instead of on a page render.
  2. getRegisteredModules() at the end of install() - sweeps modules registered before psgdpr.
  3. addModuleConsent() rewritten to write through the legacy Db layer instead of the Doctrine
    repositories.

Both of (1) and (2) are needed: stock id_module ordering is productcomments=29, psgdpr=31,
contactform=56, ps_emailalerts=72 - the first is reachable only by the sweep, the last two only by the
listener.

Why (3) - this is the part that took two attempts

The first version kept Doctrine and added a null guard, reasoning that Module::get() returns null without
a container. That guard is not sufficient, and the install sweep built on it was actively harmful:

Module::get() catches only ContainerNotFoundException. When a container exists but was compiled before
this module was installed, LoadServicesFromModulesPass (which only iterates
prestashop.installed_modules) never registered the module's services, so get() throws
ServiceNotFoundException
- before any guard can inspect the result, and not a PrestaShopException that
install() could catch.

Measured on the real path (uninstall psgdpr, drop the compiled container, install):

guard version:      container has ConsentRepository while psgdpr is NOT installed: false
                    install() THREW ServiceNotFoundException
                    consent rows created: 0        module left half-installed (installed=true)

legacy-Db version:  install() returned: true   errors: []
                    consent rows created: 4  (4 consents x 2 language rows)
                    FO /contact-us 64,693 bytes, checkbox present, 0 fatals

The rewrite also makes the listener work where the old one could only bail out:

container present:  contactform unregister -> register  =>  1 consent row
no container at all (legacy CLI):                        =>  1 consent row

Verification notes

  • Driven through a booted AdminKernel with $context->container set the way a BO controller supplies it,
    and through $module->install() in a separate request after clearing var/cache/dev, so the container
    is genuinely compiled without psgdpr - that is the condition that made the first attempt fail.
  • Test artefact worth not repeating: Hook::getHookModuleExecList() builds its static cache for ALL
    hooks at the first Hook::exec() of the request. Registering psgdpr on a hook and then triggering that
    hook in the SAME request shows the listener never firing - the cache, not the code. That edge remains for
    "upgrade psgdpr and install a consumer in one request", where the config-page sweep is the backstop.
  • Shop restored to baseline afterwards: psgdpr 1.4.3, 5 hook rows, 0 consent rows, /contact-us back to
    58,177 bytes, core working tree clean.

Gates

  • php -l on both changed PHP files - clean. Added syntax (private const, : void) is 7.1+, so the 7.2
    CI matrix leg is safe.
  • php-cs-fixer with the module's own .php-cs-fixer.dist.php - clean.
  • PHPStan level 5 with tests/phpstan/phpstan-9.2.x.neon - psgdpr.php 0 errors, unchanged from baseline
    (250 pre-existing errors live in src/ and controllers/, untouched). The local run has no core classes
    wired, so CI stays authoritative.

Coordination

  • PR Improve performances - do not register modules all the time when loading the configuration page #235 (PululuK/Evolutive, open) makes getRegisteredModules() run less often on the config page.
    Complementary - this change removes the reason the page-render sweep has to be the creation path at all -
    but it also adds upgrade/upgrade-2.0.4.php, a direct filename collision to resolve at publish
    (renumber whichever lands second).
  • PR Include id_shop in the primary key on table psgdpr_consent_lang #230 (lmeyer1, open) puts id_shop in the psgdpr_consent_lang primary key. The rewritten insert
    writes id_shop exactly as the Doctrine path did ($this->context->shop->id, one lang row per language),
    so it adds no new scoping and does not make multistore worse. The pre-existing asymmetry -
    findModuleConsentExist()/getConsentActive() do not filter shop while checkIfExist() does - is Include id_shop in the primary key on table psgdpr_consent_lang #230's
    territory.
  • Delivery gap: core 9.2 pins "prestashop/psgdpr": "^1.4" -> v1.4.3 (Dec 2022) while dev is 2.0.3,
    so a dev fix reaches no 9.2 shop until core bumps that constraint, and no 1.4.x maintenance line exists
    (1.4.3 is a release snapshot). Recorded as DECISIONS A7 rather than acted on unilaterally.

Review risk to expect

The module is migrating to Doctrine, and (3) moves one write back to the legacy layer. The justification is
mechanical and measured - no container exists at the two call sites that matter - but a maintainer may prefer
a different shape (e.g. a container-free repository, or deferring creation to the first BO request). Worth
leading the PR description with the install() THREW ServiceNotFoundException measurement.

… page renders

The consent row that gates every front-office consent checkbox was only ever
created as a side effect of rendering the module configuration page:
getRegisteredModules() is called from getContent() and nowhere else. Until an
employee opened that page, no row existed for contactform, ps_emailsubscription,
productcomments or ps_emailalerts, so hookDisplayGDPRConsent() returned an empty
string and no checkbox was shown anywhere.

Create the row from actionModuleRegisterHookAfter, when a module declares itself
on registerGDPRConsent, and sweep the already-registered modules on install.
Both are needed: on a stock shop productcomments is installed before psgdpr and
only the sweep reaches it, while contactform and ps_emailalerts come after and
only the listener does.

addModuleConsent() now writes through the legacy layer rather than the Doctrine
repositories, because neither of those call sites can reach them. During a
module install the container has been compiled before that module was installed,
so LoadServicesFromModulesPass never registered its services and
$this->get() throws ServiceNotFoundException; under the shop installer there is
no container at all and it returns null. Neither is a PrestaShopException, so
install() cannot catch either one.
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.

GDPR - Module Consent checkbox customization no checkbox in FO for Account creation form/Newsletter subscription/product Comments/Contact Form

1 participant