mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 09:26:52 +01:00
Refine events articles after review feedback
Correct wording in five events articles to reflect that AL event subscribers bind by parameter name, not position: - add-new-event-parameters-at-the-end: drop the inaccurate claim that appending a parameter forces subscribers to be updated or causes wrong values; keep the append-at-end best practice. - do-not-add-ishandled-to-an-existing-event: reframe from "breaking change" to the semantic/purpose shift that leaves existing subscribers pointless; rename the breaking-change keyword to semantic-change. - name-events-by-publisher-position: extend the good sample with position-named publishers raised from table and report trigger contexts. - initialize-ishandled-to-false-before-publishing: scope the detection and best practice to events that actually carry a var IsHandled, so an OnBefore with no IsHandled is not flagged. - do-not-bypass-critical-operations-with-ishandled: add a litmus-test definition of a critical operation (code that cannot stand as an independent, self-contained unit). Knowledge-only; no contract or wiring change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
parent
faeacb2484
commit
0ad92cad86
5 changed files with 35 additions and 7 deletions
|
|
@ -11,7 +11,7 @@ application-area: [all]
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
Adding a parameter to an existing event publisher changes its signature, and every subscriber must be updated to match. Appending the new parameter at the end of the parameter list keeps the change easy to review and minimizes churn: existing subscribers still bind to the leading parameters, and the diff is a single clean addition. Inserting a parameter in the middle shifts every following argument, makes diffs noisy, and is error-prone to reconcile across many subscribers — a subscriber that compiles can still receive the wrong values because positions moved. New parameters belong after the existing ones.
|
Adding a parameter to an existing event publisher changes its signature. Appending the new parameter at the end of the parameter list keeps the change easy to review and track: existing subscribers still bind to the leading parameters, and the diff is a single clean addition. Inserting a parameter in the middle makes diffs noisy and harder to review, and obscures the history of how the signature evolved. New parameters belong after the existing ones.
|
||||||
|
|
||||||
## Best Practice
|
## Best Practice
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,7 +1,7 @@
|
||||||
---
|
---
|
||||||
bc-version: [all]
|
bc-version: [all]
|
||||||
domain: events
|
domain: events
|
||||||
keywords: [ishandled, breaking-change, event-contract, backward-compatibility, onbefore, integration-event, subscribers]
|
keywords: [ishandled, semantic-change, event-contract, backward-compatibility, onbefore, integration-event, subscribers]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
countries: [w1]
|
countries: [w1]
|
||||||
application-area: [all]
|
application-area: [all]
|
||||||
|
|
@ -11,7 +11,7 @@ application-area: [all]
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
Adding a `var IsHandled: Boolean` parameter to an event that already shipped without one is a breaking contract change. The signature changes, so existing subscribers no longer match and silently stop firing until they are updated, and the event's meaning shifts from "notify" to "overridable" — a semantic the original subscribers never agreed to. The safe move is to leave the existing event untouched and introduce a new `OnBefore…` event carrying `IsHandled` at the point you want to make overridable. Existing subscribers keep working against the original event; new subscribers opt into the override seam through the new one.
|
Adding a `var IsHandled: Boolean` parameter to an event that already shipped without one silently changes the event's purpose — from a plain notification into an overridable seam. Existing subscribers were written against a "notify" contract they never agreed to make skippable, so their behaviour can quietly become wrong or pointless. The safe move is to leave the existing event untouched and introduce a new `OnBefore…` event carrying `IsHandled` at the point you want to make overridable. Existing subscribers keep working against the original event; new subscribers opt into the override seam through the new one.
|
||||||
|
|
||||||
## Best Practice
|
## Best Practice
|
||||||
|
|
||||||
|
|
@ -21,6 +21,6 @@ See sample: `do-not-add-ishandled-to-an-existing-event.good.al`.
|
||||||
|
|
||||||
## Anti Pattern
|
## Anti Pattern
|
||||||
|
|
||||||
Mutating a shipped event — for example adding `var IsHandled` to `OnAfterCalculateTotal` — to retrofit override behaviour, breaking every existing subscriber and overloading the event's meaning. Detection: an `IsHandled` parameter added to a pre-existing event signature rather than introduced through a new dedicated `OnBefore` publisher.
|
Mutating a shipped event — for example adding `var IsHandled` to `OnAfterCalculateTotal` — to retrofit override behaviour, which overloads the event's meaning and undermines existing subscribers. Detection: an `IsHandled` parameter added to a pre-existing event signature rather than introduced through a new dedicated `OnBefore` publisher.
|
||||||
|
|
||||||
See sample: `do-not-add-ishandled-to-an-existing-event.bad.al`.
|
See sample: `do-not-add-ishandled-to-an-existing-event.bad.al`.
|
||||||
|
|
|
||||||
|
|
@ -11,7 +11,7 @@ application-area: [all]
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
The IsHandled override pattern lets a subscriber skip the guarded code entirely. That is acceptable around a pure, side-effect-free calculation, but dangerous around critical operations — posting, ledger-entry creation, number-series consumption, and referential-integrity or permission validation. Wrapping those in `OnBeforeX(…; var IsHandled); if IsHandled then exit;` lets any subscriber silently suppress them, risking imbalanced ledgers, orphaned documents, skipped permission checks, or duplicated numbers — corruption that surfaces far from the subscriber that caused it. Make the calculation overridable, not the commit: expose the value computation through IsHandled, or offer a regular `OnAfter…` event to adjust results, while the critical work runs unconditionally.
|
The IsHandled override pattern lets a subscriber skip the guarded code entirely. A critical operation is one that cannot stand as an independent, self-contained unit — code whose partial execution or omission leaves the system inconsistent (imbalanced ledgers, orphaned documents, gaps in a number series, or skipped permission checks). That is acceptable around a pure, side-effect-free calculation, but dangerous around critical operations — posting, ledger-entry creation, number-series consumption, and referential-integrity or permission validation. Wrapping those in `OnBeforeX(…; var IsHandled); if IsHandled then exit;` lets any subscriber silently suppress them, risking imbalanced ledgers, orphaned documents, skipped permission checks, or duplicated numbers — corruption that surfaces far from the subscriber that caused it. Make the calculation overridable, not the commit: expose the value computation through IsHandled, or offer a regular `OnAfter…` event to adjust results, while the critical work runs unconditionally.
|
||||||
|
|
||||||
## Best Practice
|
## Best Practice
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -15,12 +15,12 @@ A routine that raises an `OnBefore…` integration event with a `var IsHandled:
|
||||||
|
|
||||||
## Best Practice
|
## Best Practice
|
||||||
|
|
||||||
Set `IsHandled := false;` immediately before each `OnBeforeX(…, IsHandled)` raise, then guard the default logic with `if IsHandled then exit;` or `if not IsHandled then …`. Do this even when the variable was just declared: the explicit reset documents intent and stays correct if a second event raise is added to the routine later.
|
Set `IsHandled := false;` immediately before each `OnBeforeX(…, IsHandled)` raise, then guard the default logic with `if IsHandled then exit;` or `if not IsHandled then …`. Do this even when the variable was just declared: the explicit reset documents intent and stays correct if a second event raise is added to the routine later. This applies only to events that carry a `var IsHandled: Boolean`; an `OnBefore` event with no `IsHandled` parameter needs no reset.
|
||||||
|
|
||||||
See sample: `initialize-ishandled-to-false-before-publishing.good.al`.
|
See sample: `initialize-ishandled-to-false-before-publishing.good.al`.
|
||||||
|
|
||||||
## Anti Pattern
|
## Anti Pattern
|
||||||
|
|
||||||
Raising `OnBeforeX(…, IsHandled)` with a variable whose value carries over from an earlier raise, so a subscriber that handled the first event unintentionally suppresses the second routine's default logic. Detection: an `IsHandled` variable passed to more than one event in a routine without an intervening `IsHandled := false;`, or any `OnBefore…` raise not preceded by an explicit reset.
|
Raising `OnBeforeX(…, IsHandled)` with a variable whose value carries over from an earlier raise, so a subscriber that handled the first event unintentionally suppresses the second routine's default logic. Detection: an `IsHandled` variable passed to more than one event in a routine without an intervening `IsHandled := false;`, or any `OnBefore…` raise that passes an `IsHandled` variable without an intervening `IsHandled := false;`.
|
||||||
|
|
||||||
See sample: `initialize-ishandled-to-false-before-publishing.bad.al`.
|
See sample: `initialize-ishandled-to-false-before-publishing.bad.al`.
|
||||||
|
|
|
||||||
|
|
@ -33,4 +33,32 @@ codeunit 50255 "Event Naming Good Sample"
|
||||||
local procedure OnAfterPostSalesLine(var SalesLine: Record "Sales Line")
|
local procedure OnAfterPostSalesLine(var SalesLine: Record "Sales Line")
|
||||||
begin
|
begin
|
||||||
end;
|
end;
|
||||||
|
|
||||||
|
// Same position-naming convention applies to events raised from table and
|
||||||
|
// report triggers, not just codeunit procedures.
|
||||||
|
|
||||||
|
// Raised at the end of a table field's OnValidate trigger (for example
|
||||||
|
// Customer."No." OnValidate): the position is "after", so OnAfter<Field>.
|
||||||
|
procedure HandleCustomerNoValidated(var Customer: Record Customer)
|
||||||
|
begin
|
||||||
|
OnAfterValidateCustomerNo(Customer);
|
||||||
|
end;
|
||||||
|
|
||||||
|
[IntegrationEvent(false, false)]
|
||||||
|
local procedure OnAfterValidateCustomerNo(var Customer: Record Customer)
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
|
||||||
|
// Raised before a report prints a line from its processing trigger (for
|
||||||
|
// example a dataitem OnAfterGetRecord): the position is "before", so
|
||||||
|
// OnBefore<Action>.
|
||||||
|
procedure HandleReportLineProcessing(var SalesLine: Record "Sales Line")
|
||||||
|
begin
|
||||||
|
OnBeforeReportPrintLine(SalesLine);
|
||||||
|
end;
|
||||||
|
|
||||||
|
[IntegrationEvent(false, false)]
|
||||||
|
local procedure OnBeforeReportPrintLine(var SalesLine: Record "Sales Line")
|
||||||
|
begin
|
||||||
|
end;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue