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
Draft
Conversation
… 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.
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_psgdpr_consentholds one row per module that wants a consent checkbox, andhookDisplayGDPRConsent()renders nothing unlessGDPRConsent::getConsentActive($id_module)finds one. The only code that creates those rows isgetRegisteredModules(), whose only caller isgetContent()- 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-uscarries 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.ps_psgdpr_consentis 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_consentholds one row per module that wants a consent checkbox.hookDisplayGDPRConsent()returns
''unlessGDPRConsent::getConsentActive($id_module)finds one. The only code that creates thoserows is
getRegisteredModules(), and it is called fromgetContent()and nowhere else - i.e. as a sideeffect 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:
dev(2.0.3) has the identical shape:getRegisteredModules()still hasgetContent()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 andsubmitDataConsent()cannot writeactive=0by omission. Said so in the upstream comment rather thanletting the tidier story absorb the discrepancy.
zodit/Miguel86/carlos-blazquez describe a different cause with the same symptom (psgdpr missing from the
displayGDPRConsenthook);upgrade/upgrade-1.4.3.phpalready repairs that one.The fix
actionModuleRegisterHookAfterlistener - creates the row when a module declares itself onregisterGDPRConsent, instead of on a page render.getRegisteredModules()at the end ofinstall()- sweeps modules registered before psgdpr.addModuleConsent()rewritten to write through the legacyDblayer instead of the Doctrinerepositories.
Both of (1) and (2) are needed: stock
id_moduleordering 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()returnsnullwithouta container. That guard is not sufficient, and the install sweep built on it was actively harmful:
Module::get()catches onlyContainerNotFoundException. When a container exists but was compiled beforethis module was installed,
LoadServicesFromModulesPass(which only iteratesprestashop.installed_modules) never registered the module's services, soget()throwsServiceNotFoundException- before any guard can inspect the result, and not aPrestaShopExceptionthatinstall()could catch.Measured on the real path (uninstall psgdpr, drop the compiled container, install):
The rewrite also makes the listener work where the old one could only bail out:
Verification notes
AdminKernelwith$context->containerset the way a BO controller supplies it,and through
$module->install()in a separate request after clearingvar/cache/dev, so the containeris genuinely compiled without psgdpr - that is the condition that made the first attempt fail.
Hook::getHookModuleExecList()builds its static cache for ALLhooks at the first
Hook::exec()of the request. Registering psgdpr on a hook and then triggering thathook 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.
/contact-usback to58,177 bytes, core working tree clean.
Gates
php -lon both changed PHP files - clean. Added syntax (private const,: void) is 7.1+, so the 7.2CI matrix leg is safe.
php-cs-fixerwith the module's own.php-cs-fixer.dist.php- clean.tests/phpstan/phpstan-9.2.x.neon-psgdpr.php0 errors, unchanged from baseline(250 pre-existing errors live in
src/andcontrollers/, untouched). The local run has no core classeswired, so CI stays authoritative.
Coordination
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).
id_shopin thepsgdpr_consent_langprimary key. The rewritten insertwrites
id_shopexactly 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 whilecheckIfExist()does - is Include id_shop in the primary key on table psgdpr_consent_lang #230'sterritory.
"prestashop/psgdpr": "^1.4"-> v1.4.3 (Dec 2022) whiledevis 2.0.3,so a
devfix reaches no 9.2 shop until core bumps that constraint, and no 1.4.x maintenance line exists(
1.4.3is 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 ServiceNotFoundExceptionmeasurement.