This pull request seems to contain new translation strings. I have summarized them below to ease up review:

• Admin.Advparameters.Notification
  • Another extra property already stores its values in the same database column. Use the same entity name as the existing property, or a different property name.
  • The default value does not match the field type (or is not one of the allowed choice values).
• Admin.Advparameters.Feature
  • Entity name
• Admin.Advparameters.Help
  • Entity or ObjectModel table name (e.g. product, customer, order, combination, attribute).

(Note: this is an automated message, but answering it will reach a real human)

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

• The branch should be develop, 9.1.x, 9.0.x or 8.2.x. (Read explanation)

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

(Note: this is an automated message, but answering it will reach a real human)

Recap of my review above, since the blocking point was only stated in prose. Only point 1 blocks the merge.

1. Blocking. ExtraPropertyRegistry::hasStorageChanges() compares type, scope, size, nullable and enum values, but not the table. Since the new STORAGE_CONFLICT guard is gated on null === $existingDefinition, re-registering an existing definition with a different explicit tableName passes every check. Reproduced: store to supplier, the original column keeps its data but becomes orphaned, the definition then reads an empty table, and nothing warns. Same class as a scope change, which the registry already refuses for exactly that reason. $incoming->getTableName() !== $existing->getTableName() closes it.

2. Needs a decision rather than a fix. Those three entities have a real Symfony grid but derive to a tab that does not exist (checked against ps_tab), so a BOOL extra property toggle is permanently 403. Not a regression, the previous string-based version derived AdminAttributes too. What tips it is that the same commit widens the BO help text to include attribute, whose toggle cannot work.

3. Nit. The doc committed in this PR describes the override map that its own last commit removed.

4. Nit. The docblock states hydration never re-resolves, while the constructor calls resolveObjectModelDefinition() unconditionally before the ?? chain and discards the result.

Everything else held up: the naming, default typing and cart work are verified end to end locally (details in the review), 609 unit / 106 integration / 22 Behat scenarios green, and CI is green.

Re-tested f5a242e locally. The shared rule-set is the right move, and the DATE tightening lands exactly where Cyril and you converged. Verified on the registry:

refused  : tomorrow           refused  : now
accepted : 2026-03-12         accepted : 2026-03-12 08:30:00
refused  : 2026-02-31         refused  : 12/03/2026

2026-02-31 being refused is worth calling out: the round-trip format comparison in isLiteralDateTime() catches the roll-over that createFromFormat() alone would let through as March 3rd. Nice.

My observation (d) from the previous comment is fully closed too: the docblock now describes what the code does, and an unknown definition returns the same 403 as a denied one, so the probe is gone.

One new point: the "every regular write" claim only holds on the Admin API

isDefaultValueCompatible() docblock:

the SAME isValueCompatible() rule-set the validator applies to every regular write, so what is refused as a value is refused as a default and vice versa

and validateValue():

so a value that could not be a default is not storable either (no silent coercion — 'abc' on an INT used to be stored as 0, 'tomorrow' on a DATE was interpreted)

The only consumer of ExtraPropertyValidatorInterface in the whole repository is ExtraPropertyCQRSApiValidator. The BO entity form runs the definition's declared constraints only, and the ObjectModel / module-code path runs nothing at all, so neither goes through isValueCompatible().

On the API path the rule does bite, as intended:

qa_int  <- 'abc'       violations=1  "The value is not compatible with the declared "int" field type."
qa_date <- 'tomorrow'  violations=1  "The value is not compatible with the declared "date" field type."

The same two values through ExtraPropertyWriter::writeAll(), which is what $product->extra_properties[...] = ... and any module write end up calling:

INT  'abc'      -> silently coerced to 0
                   bound params: [1, 0, "tomorrow"]
DATE 'tomorrow' -> SQLSTATE[22007] 1292 Incorrect datetime value: 'tomorrow' for column 'qa_date'
                   raw Doctrine\DBAL\Exception\DriverException out of writeCommon()

So both symptoms the comment says are gone survive outside the API: 'abc' still becomes 0 silently, and 'tomorrow' is only "no longer interpreted" in the sense that MySQL rejects it with a 500 rather than a domain error. The default value hole is genuinely closed; the write hole is closed on one surface out of three.

Two ways to square it, your call:

• Docblock only. Reword both comments to say the rule-set is shared between defaults and the Admin API write path, and that other paths keep their existing coercion. Cheapest, and honest.
• Apply it at the choke point. writeAll() is already the single castForDb() choke point this PR established, so calling isValueCompatible() there (per definition, per leaf for the LANG/SHOP array shape, before the cast) would turn that DBAL 500 into a clean refusal on every path and make the docblock true as written. It does widen the PR though, and it is a behaviour change for module code that currently relies on the coercion, so a follow-up is a perfectly reasonable answer.

I have no strong preference between the two, I just would not leave the docblock claiming the broad version while the narrow one is what ships.

Everything else from my earlier comments still checks out on this head, and the suites stay green locally (unit 682, integration 108, behat 22 scenarios / 106 steps). Points (a) tax_rule and (b) combination from the previous comment are still open as far as I can tell. Wrong, corrected: both were already fixed in f5a242e, along with the singular-tab family from point (c). I had diffed only two files of that commit and missed the map additions in ExtraPropertyDefinition.php.

Hello @jolelievre,

Please note that the “Needs autoupgrade PR” label has been automatically added to your Pull Request. This means that the changes made in your PR must also be reported in the Autoupgrade module, responsible for running the update process of PrestaShop stores.

When there's a modification to the db-structure.sql file or if a Doctrine entity has been modified, added, or deleted, it impacts the database structure. Modifying the list of hooks and feature flags has consequences on the update process as well. These changes can affect how the application interacts with the database and how update procedures are handled. It's important to consider these aspects when contributing to ensure a smooth update experience for users.

That's why this contribution should be backported, with another corresponding PR, on the Autoupgrade module's GitHub repository which is available here: Autoupgrade Repository
Contribution to the Autoupgrade module documentation: Documentation Link

Don't forget to add a comment in this current thread with the link to this PR once it has been created.

If you are sure that your Pull Request doesn’t require additional actions, ping committers and it will be checked.

Thank you.

Thanks @mattgoud Good catch on the coverage — two different situations though:

• BO forms: this was a real gap, fixed by a new ExtraPropertyTypeCompatibility constraint that the form builder modifier now attaches to every extra field (wrapped in Assert\All for localized ones), so the same isValueCompatible() rules are enforced inline.
• ObjectModel: the integration already worked — add()/update() call validateExtraProperties() before any row is written, through the same shared validator. What was missing was proof, so a dedicated ObjectModelExtraPropertiesTest now covers it end to end: add/update round-trips, LANG values across two languages, delete purge, and a type-incompatible value aborting the whole save.

So the proper solution was NOT to integrate the validation in the deeper level of the writer, because it's too late (core data already persisted at this point), but to correctly include this new default validation properly in all three layers

Correction on my previous comment: the claim was wrong, the docblock was right. Please disregard the "every regular write" point, and in particular do not reword those docblocks on my account.

ObjectModel::add() and update() do call validateExtraProperties() (classes/ObjectModel.php:559 and :759), which calls $validator->validate(), which delegates per property to validateValue(). So the ObjectModel path does go through isValueCompatible(), exactly as the comment says. My grep for consumers of ExtraPropertyValidatorInterface was scoped to src/ and the API module and missed classes/.

Both pieces of evidence I posted were bad, which is what let the wrong conclusion through:

• the "silently coerced to 0" trace came from calling ExtraPropertyWriterInterface::writeAll() directly, which bypasses ObjectModel entirely. I presented it as what $product->extra_properties[...] = ... ends up doing, which skips the validation step that runs first.
• my ObjectModel test returned "valid" for 'abc' on an INT because it never reached the validator: findService() resolves through ContainerFinder(Context::getContext()), and my script booted AdminKernel without wiring the container onto the context, so validateExtraProperties() took its documented "container unavailable" early return.

With Context::getContext()->container wired, as in any real BO request:

qa_int  = 'abc'        => REFUSED: _core.qa_int: The value is not compatible with the declared "int" field type.
qa_date = 'tomorrow'   => REFUSED: _core.qa_date: The value is not compatible with the declared "date" field type.
qa_int  = '42'         => VALID
qa_date = '2026-03-12' => VALID

and a real update() throws PrestaShopException: _core.qa_int: The value is not compatible with the declared "int" field type.

So with 83e0bbdc the rule now lands on all four surfaces: registration defaults, Admin API, ObjectModel, and the BO form inline through the new ExtraPropertyTypeCompatibility constraint. Making it a real Symfony constraint with a dependency-free validator so it also builds in the hand-built FO container is the right shape, and auto-attaching it in the form builder modifier rather than asking modules to declare it keeps the safety net non-optional.

The only two gaps left are both already documented behaviour rather than oversights: a direct writeAll() call bypasses validation (not the documented module API, which is $entity->extra_properties[...]), and FO writes skip it when the validator is absent from the legacy container, which is stated in validateExtraProperties() itself.

Sorry for the noise, and for proposing a docblock change that would have made the documentation less accurate than it already was.

QA OK

Tested on: 9.2.x, commit 83e0bbdc, local Docker, database upgraded in place (see the note at the end)
Shop: single shop
Browser: Chrome 141
Linked issues: #41779, #41829, #41424, all three have concrete steps and the PR's own "How to test" adds a manual procedure, which is what I followed.

Automated UI tests: none for this feature (UI Tests: N/A in the description) ✅ nothing to replay. CI is green (40 checks). The PHP suites pass locally: unit 685, integration 114, behat extra-property-definition 22 scenarios / 106 steps.

Steps followed

Everything below was done through the back office, following the PR's manual procedure. The demoextrafield fixtures were not installed, see coverage at the end.

1. Naming (#41779) — created three definitions from Advanced Parameters > Extra Properties, typing the entity name the way a merchant would: Combination, Order, Cart.

Each one normalised to its canonical entity and stored next to the physical base table:

controller_name is NULL on all three, so the permission subject is deduced rather than frozen, which is the intended behaviour.

2. Defaults (#41829) — the Combination property was created as Integer with default 0, the falsy value that used to be dropped.

Read back from the edit form after the round trip (DOM, since a back-office page cannot be curl-ed):

{"entity_name": {"value": "combination", "disabled": true},
 "type":        {"value": "int",         "disabled": true},
 "default_value": {"value": "0",         "disabled": true}}

and on the column itself:

ps_product_attribute_extra.qa_combo_int   int   NULL=YES   COLUMN_DEFAULT=0

So 0 survives to the registry row, to the DDL, and back onto the form. The field is rendered disabled on edit, which matches the documented immutability of defaultValue.

A default of abc on the same Integer type is refused with a message that says why, and nothing is written: the registry still holds only the three rows and no qa_bad* column was added to ps_product_attribute_extra.

3. Row-less entity reads the declared default — combination #1 has no row at all in ps_product_attribute_extra (SELECT COUNT(*) = 0), and the reader returns the declared default, typed as an int rather than a string:

{"_core":{"qa_combo_int":0}}

4. Cart (#41424) — display_front switched on for the cart property from the edit form, a product added to the cart in the front office, then a value written on that cart.

prestashop.cart in the front office, which is the CartFilter whitelist change:

// empty cart, no value stored yet
extra_properties: {"_core": {"qa_cart_note": null}}
// after writing a value on cart 6
extra_properties: {"_core": {"qa_cart_note": "Livraison le vendredi"}}

The key also appears in the server-rendered inline JSON on an anonymous request, right where it belongs in the cart object, so it is not a client-side artefact:

"minimalPurchase":0,"minimalPurchaseRequired":"","extra_properties":[]

Result

All four checks behave as the description says. Before this PR a combination registration would have stored on a combination_extra table with an id_combination column that does not exist, and a 0 default was dropped as falsy on the way in.

Regression pass

Files changed point at the registry, the reader/writer, the grids and forms, and CartFilter. Checked around them: the definitions grid lists, filters and sorts normally, the create and edit forms save and reject correctly, the front-office home and cart pages render with no console error, and the order and product grids still load. No *_extra leftovers after removing the test definitions.

Coverage note

Two steps of the recommended procedure were not run because they need the demoextrafield companion, which is a separate PR. Closed: the module is now installed and both steps are covered in a follow-up comment below, including the cart-to-order copy through the real actionValidateOrder and the combination defaults on the Admin API. Only the two front-office template renderings remain unverified, for lack of a customer session.

Environment note

The database was upgraded in place rather than reinstalled: the two new registry columns (table_name, controller_name) were added by hand with the exact types from db_structure.sql, which is what the companion autoupgrade PR does. That makes this run an exercise of the upgrade path rather than the fresh-install path. A fresh install is still worth one pass before release.

Disclosure: I also reviewed this PR, so a second pair of eyes on the QA is welcome if the process calls for it.

QA follow-up: the coverage gap from my previous comment is now closed. I installed the demoextrafield companion (2bff685) on the same shop, 9.2.x at 83e0bbdc.

The module registers cleanly, and it exercises the naming map on real module code

18 definitions across 9 entities, which is a much better surface than the three I had created by hand. The irregular ones land where they should:

controller_name is NULL on all 18, so every one of them takes the deduction. Storage columns follow the {module}_{property} convention, e.g. on ps_customer_extra: demoextrafield_credit_limit, demoextrafield_extra_json, demoextrafield_internal_note.

Cart note, on screen at the checkout

actionCartSave seeds it, it crosses CartFilter into prestashop.cart grouped under the module key, and the template renders it above the summary:

extra_properties: {"demoextrafield": {"delivery_note": "Leave the parcel at the pickup point (demo note seeded on 2026-09-07 14:55)."}}

Cart to order copy, through the real hook

New cart for the demo customer, note written through ExtraPropertyWriter (not a raw INSERT), then a real validateOrder():

cart #7   ps_cart_extra.demoextrafield_delivery_note   = 'QA: fragile, sonner chez le voisin'
validateOrder() = true  ->  order #6
order #6  ps_orders_extra.demoextrafield_delivery_note = 'QA: fragile, sonner chez le voisin'
reader on 'orders' -> {"demoextrafield":{"delivery_note":"QA: fragile, sonner chez le voisin"}}

Worth noting what this specifically proves: the write and the read both go through ps_orders_extra.id_order. Before this PR the order entity resolved to an id_orders column that does not exist, so this is the naming fix on the write path, not just on the read path.

Combination defaults on the Admin API

ps_product_attribute_extra holds 0 rows, and the list endpoint still answers:

{"combinationId": 1, "extra_demoextrafield_ean_verified": false}
{"combinationId": 2, "extra_demoextrafield_ean_verified": false}
{"combinationId": 3, "extra_demoextrafield_ean_verified": false}

A typed boolean false served purely by the default read injection, and combinationId resolved by the corrected heuristic rather than productAttributeId. This is the exact example the description gives.

I also tried a PATCH on /products/{productId}/combinations/{combinationId} and got a 404. That is not a defect: debug:router --app-id=admin-api shows only GET routes for combinations in ps_apiresources, and the module declares the list URI only. Flagging it so nobody else spends time on it.

JSON decoded on reads

Stored encoded, read back as a real structure:

in DB   : string(85) "{"loyalty_tier":"gold","since":"2024-03-01","perks":["free_shipping","early_access"]}"
reader  : array  ['loyalty_tier' => 'gold', 'since' => '2024-03-01', 'perks' => ['free_shipping', 'early_access']]

So a Smarty template iterates it, which is what the my-account rendering relies on.

What is still not covered

Two visual renderings only, both needing a logged-in front-office customer session, which I could not open myself:

• the note on the customer's own order detail page (displayOrderDetail)
• the JSON block on the my-account page (displayCustomerAccountTop)

Their data paths are both verified above (order #6 reads its note back, and the JSON comes back as an array), so what is missing is the template output, not the mechanism. Cheap for anyone with a customer session to confirm in a minute.

Verdict unchanged: QA OK.

The last two gaps are closed: both front-office renderings are now verified with a real logged-in customer session.

Cart note carried onto the customer's order

Order VWSCQTGLU, seeded by writing the note on a fresh cart through ExtraPropertyWriter and then a real validateOrder(). The customer's own order detail page shows it:

So the full chain holds on the customer-visible side, not just in the database: cart value → actionValidateOrder copy → ps_orders_extra.id_order → order detail template.

Customer JSON rendered as a structure

The value was written through the writer as a PHP array (so it went through castForDb and was encoded on the way in), and the my-account page iterates it rather than printing a raw string:

credit_limit: 250 alongside it is worth a glance too: a FLOAT read back as a number, not 250.000000.

The nested perks array prints as its JSON representation because the module template only iterates one level. That is a template choice, not a core behaviour.

URL note for whoever retests

The order detail page is reachable at ?controller=order-detail&id_order=N, not /order-detail?id_order=N, which 404s on this install. I mention it only because I lost a step to it.

Coverage is now complete

Every step of the recommended manual procedure has been run, plus the Admin API default read and the registry/DDL checks from my earlier comments. Verdict stays QA OK.

Environment restored afterwards: module uninstalled and removed, the 12 storage tables it leaves behind on purpose ($dropColumn = false in its own uninstall()) dropped by hand, QA order and carts deleted, API client removed, PS_ADMIN_API_FORCE_DEBUG_SECURED back to 1.

Hi @jolelievre, thanks for linking my earlier PR #42343 in the description.

I compared the diffs and noticed that the CQRS changes I submitted on August 10 appear in this merged PR, including the scalar union types in AddExtraPropertyDefinitionCommand and EditableExtraPropertyDefinition, and the removal of the string cast in GetExtraPropertyDefinitionForEditingHandler.

I appreciate the link to my PR, but I’m trying to understand the attribution: was the absence of author or co-author credit on the merged change an oversight, or an intentional choice because this PR expanded the work? Could you clarify how you incorporated #42343 and what attribution you think is appropriate?