Add the Extra Properties component documentation - #2169
Conversation
mattgoud
left a comment
There was a problem hiding this comment.
Thorough page. I checked it against 9.2.x and ps_apiresources dev. The argument table, enums, registry codes, placement grammar, constraint allowlist, BO page, grid hooks and the definitions endpoints all match. Five points to fix, three of them reproduced on a 9.2.x shop with demoextraproperty installed.
Should fix
register-from-module.md:100:registerExtraProperty()does not always returnfalse, the constructor exceptions listed at:92can come out of it too.Module::registerExtraProperty()callswithModuleName()before itstry(classes/module/Module.php:1255vs:1261). The "domain must belong to the module" check only runs once the module name is set, so a definition built withoutmoduleName(the documented pattern) and a foreignlabelDomainmakes it throw. Reproduced:labelDomain: 'Modules.Othermodule.Admin'throwsInvalidExtraPropertyDefinitionException("labelDomain … of a module-owned definition must belong to the module"). Thecatch (ExtraPropertyRegistryException)sample at:108-114does not catch it either. I opened PrestaShop/PrestaShop#43017 to fix it in the core (move the injection inside thetry,unregisterExtraProperty()has the same shape). Until it lands, the page should not promisefalsefor that case.register-from-module.md:160lists "Scope change" underDESTRUCTIVE_SCHEMA_CHANGE. The scope is checked first and refused withSCOPE_CONFLICT(ExtraPropertyRegistry.php:133). Reproduced: re-registering acommonproperty asshopreturnsfalsewith "already registered with scope "common", cannot also register with scope "shop"". The codes table at:120is right._index.md:111:{$customer->extra_properties…}is the one variable name that fails on the Front Office. The$customerglobal is a presented array. Rendered with Smarty against a presented customer:{$customer->extra_properties.demoextraproperty.qa_note}raises "Attempt to read property "extra_properties" on array", while{$customer.extra_properties…}and{$customerObjectModel->extra_properties…}both render the value. Suggestion: use another variable name in the raw ObjectModel example, and add the$customerglobal to the list at:104, sinceObjectPresenteraddsextra_propertiesto it too. On the same line, "Order detail" exposes the order's properties (OrderDetailLazyArrayusesOrder::class), so "Order (also on the order detail page)" is more accurate._index.md:100: "only the Front Office filters ondisplayFront" misses whatContext::isFrontOfficeContext()counts as Front Office: everything that is not a BO/API controller or CLI, so the legacy webservice and HTTP-run crons or scripts too (classes/Context.php:313-342). It is worth one sentence here and in thedisplayFrontrow (register-from-module.md:75).register-from-back-office.md:112: a<!-- TODO screenshot: … -->is left.
Nits (optional)
_index.md:189: "anINTmust be numeric", but it must be an integer (^-?\d+$),"1.5"is refused._index.md:255: a table without an ObjectModel works too, the table falls back to the entity name.- The warning that
requiredadds no server-side check appears three times. One place plus links would be enough.
|
Thanks for the review, all points applied in ddb4cbe except these:
About point 3, the Front Office templates section is rewritten: presented arrays (lazy array presenters and |
|
Point 5 is done in dc9a659: the TODO comment is gone, replaced with a screenshot of the View page of a module-owned property taken on a multistore shop ( |
New section
development/components/extra-properties/:install()with everyExtraPropertyDefinitionargument, error handling, uninstall with or without dropping data, conflicts, re-registration on upgrade, reading and writing values, migrating from a custom extra table. Examples come from the demoextraproperty example module./extra-property-definitionsAdmin API endpoints.The notice in
modules/core-updates/9.2.mdnow links to the new section, and the example module link points to its new namedemoextraproperty.