microsoft / AL

Home of the Dynamics 365 Business Central AL Language extension for Visual Studio Code. Used to track issues regarding the latest version of the AL compiler and developer tools available in the Visual Studio Code Marketplace or as part of the AL Developer Preview builds for Dynamics 365 Business Central.
MIT License
732 stars 243 forks source link

ClearAll should not clear variables from other apps/base #6499

Open clasalal opened 3 years ago

clasalal commented 3 years ago

Describe the bug The issue #5730, although was closed, it goes on badly in bc17 and bc18 beta (18.0.22371.0).

To Reproduce Steps and to reproduce the behavior:

  1. Go to '...' Pageextension that extendes de "Purchase Invoice Statistics" page.

    var
        HasLicense: Boolean;
    
    trigger OnOpenPage()
    var
        cuLicenseManagement: Codeunit "IDPIRPF License Management";
    begin
        //Gestión licencia producto
        HasLicense := cuLicenseManagement.CheckLicense_Bool();
    end;
    
    trigger OnAfterGetCurrRecord()
    begin
        // Si no se tiene licencia no se hace nada
        if not HasLicense then
            exit;
    
        Rec.CalcFields("IDPIRPF IRPF Amount");
    end;

Expected behavior It is already explained with detail in issue xxx #5730.

We assign value to HasLicense variable in OnOpenPage and, when the OnAfterGetRecord is invoked, the HasLicense variable that was assigned in the OnOpenPage, has lost its value. The ClearAll function that is executed in the OnAfterGetRecord of Purchase Invoice Statistics page is also emptying the existing variables in pageextension, which was not the case in bc15 (where the same ClearAll was also on the same page) and earlier. It started to fail at bc16 and continues failing at bc17 and bc18.

As far as I know this is dangerous, because it means that ClearAll is clearing all global variables, regarding of scope. We can try to overcome this particular problem in the Purchase Invoice Statistics, Purch. Credit Memo Statistics, Sales Invoice Statistics and Sales Credit Memo Statistics by getting the value that we want in every OnAfterGetRecord (performance be damned), but it is a bit worrying that the base AL code (and maybe other extensions?) can break our apps by calling this method.

Screenshots If applicable, add screenshots to help explain your problem.

5. Versions:

dzzzb commented 3 years ago

That issue didn't provide any repro code sample/project either though, which won't help chances of MS looking at it sooner.

clasalal commented 3 years ago

Added example code.

dzzzb commented 3 years ago

I can imagine this being tricky, as surely some callers of ClearAll() really do mean 'reset this page to its exact initial/zero state', rather than 'reset only the variables declared by the extension in which I am declared'. To such callers, the old behaviour would've been a bug.

A bug in ClearAll(), that is... Left behind is the fact that this page calls ClearAll() on itself, which seems like a terrible design. That seems like a bug in the base app, not ClearAll().

demiliani commented 3 years ago

I can confirm that this bug is still present: https://demiliani.com/2021/03/05/dynamics-365-business-central-clearall-is-evil-again/ I've added here a code for a repro. BugClearAll_SD.zip

BardurKnudsen commented 3 years ago

In the concrete scenario, you could either move the license check code to the OnAfterGetCurrentRecord or move the code and state variable out into a single-instance codeunit. If the license check is done more than once in a session, you could get better performance from moving it into a single instance codeunit as you would only do the check once.

For the usage of ClearAll(), I am not sure I entirely agree with the rest of the audience: I would expect ClearAll() to clear all variables in the entire object, also extended, in pages, reports and records. I would also expect the table extension fields to have their values cleared after a ClearAll(). Or if I want to run a report repeatedly; I would want the extension elements to be cleared as well. Exactly because the extension objects aren't notified about the ClearAll(). At least, if they were notified with an OnClearAllOnBaseObject() event, they would know whether they were nuked - or, if ClearAll only worked on the base object, they could decide what to do. Are we 100% sure that we will not get error reports about extensions are not cleared when a ClearAll is executed in the base object?

dzzzb commented 3 years ago

https://github.com/microsoft/AL/issues/4862 suggests MS are considering obsoleting ClearAll() for SaaS development, which IMO makes sense given the many perils already discussed.